Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(NcDialog): New component (moved from @nextcloud/dialogs) #4550

Merged
merged 2 commits into from Oct 17, 2023
Merged

feat(NcDialog): New component (moved from @nextcloud/dialogs) #4550

merged 2 commits into from Oct 17, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 20, 2023 •
edited

☑️ Resolves

Introduces NcDialog as a generic base Dialog component, while NcModal is simply a base for modals like the viewer, NcDialog provides a convenient interface to define dialogs (see example).
The new FilePicker of the @nextcloud/dialogs package is based on top of it.

🖼️ Screenshots

One example dialog:

image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews feature: dialog Related to the dialog component labels Sep 20, 2023
@susnux susnux mentioned this pull request Sep 20, 2023
Closed
2 tasks
@szaimen
Copy link
Contributor

szaimen commented Sep 20, 2023

Just tested and there are some issues on mobile:
Screenshot_20230920_184616_Brave

@susnux
Copy link
Contributor Author

susnux commented Sep 20, 2023 •
edited

Seems to be browser related as your browser overlays the bar with the content?
Screen Shot 2023-09-20 at 19 55 45

@szaimen
Copy link
Contributor

szaimen commented Sep 20, 2023

Still the buttons should not appear at the very bottom. Is this possible?

@susnux
Copy link
Contributor Author

susnux commented Sep 20, 2023

Still the buttons should not appear at the very bottom. Is this possible?

Are you sure? I think it is more intuitive to have them on the bottom on mobile.

Firefox Chrome
Screenshot_20230920_233055_Firefox Screenshot_20230920_233223_Chrome

@raimund-schluessler

This comment was marked as resolved.

Sign in to view
@susnux

This comment was marked as resolved.

Sign in to view
@raimund-schluessler

This comment was marked as resolved.

Sign in to view
@susnux
Copy link
Contributor Author

susnux commented Sep 21, 2023 •
edited

@szaimen would you mind if we do a design review session later (maybe schedule it with the design team) and for the beginning just merge it as is?
Because there are quite a lot of other task depending on the dialog component.

BTW on material dialogs look like this on mobile:
image

@susnux
Copy link
Contributor Author

susnux commented Oct 9, 2023

@szaimen reworked the small dialogs, now it looks like normal dialogs:
image

@susnux susnux force-pushed the feat/nc-dialog-v2 branch 3 times, most recently from 7e29abc to bd05641 Compare October 9, 2023 10:30
szaimen
szaimen approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now from design perspective :)

However I guess the a11y team should look over it as it might not be 100% accessible?
cc @JuliaKirschenheuter @ShGKme @Pytal

See e.g.
image

@susnux
Copy link
Contributor Author

susnux commented Oct 13, 2023

See e.g. image

I think this is a regression of the double outline of the button focus-visible. We probably should add a margin of 4px to all buttons to ensure there is space for showing the outline.

@susnux
Copy link
Contributor Author

susnux commented Oct 13, 2023

Nevertheless fixed this for now here by adding a 4px margin to the actions (while still keeping the padding consistent):

Screenshot_20231013_163004

@susnux
feat(NcDialog): Implement a simple dialog component based on the `NcM…
da52582 
…odal` component

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
fix(NcAppSettingsDialog): Migrate to use the new NcDialog component…
b1164ed 
… as the base

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux requested a review from Pytal October 17, 2023 00:09
@susnux susnux merged commit e793851 into master Oct 17, 2023
15 checks passed
@susnux susnux deleted the feat/nc-dialog-v2 branch October 17, 2023 10:37
This was referenced Oct 21, 2023
Closed
Merged
@Antreesy Antreesy mentioned this pull request Jan 12, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artonge artonge artonge approved these changes

@szaimen szaimen szaimen approved these changes

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@ShGKme ShGKme Awaiting requested review from ShGKme

@Pytal Pytal Awaiting requested review from Pytal

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement New feature or request feature: dialog Related to the dialog component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AppSettingsDialog does scroll the section list when clicking on entry Filepicker/dialogs component
4 participants
@susnux @szaimen @raimund-schluessler @artonge