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.

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

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

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 20, 2023

☑️ 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
@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

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.

@susnux

This comment was marked as resolved.

@raimund-schluessler

This comment was marked as resolved.

@susnux
Copy link
Contributor Author

susnux commented Sep 21, 2023

@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
Copy link
Contributor

@szaimen szaimen left a comment

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 susnux force-pushed the feat/nc-dialog-v2 branch from bd05641 to 745dc22 Compare October 13, 2023 14:29
@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 susnux force-pushed the feat/nc-dialog-v2 branch from 745dc22 to b1164ed Compare October 17, 2023 00:09
@susnux susnux requested a review from Pytal October 17, 2023 00:09
@susnux susnux merged commit e793851 into master Oct 17, 2023
14 checks passed
@susnux susnux deleted the feat/nc-dialog-v2 branch October 17, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: dialog Related to the dialog component
Projects
None yet
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