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

[BITV] 9.1.4.11/4.4 - Note: The "Close" button of the modal dialog "New album" is not visually prominent. It would be better to place the button directly within the dialog instead of outside of it. (1) #2174

Closed
JuliaKirschenheuter opened this issue Nov 29, 2023 · 9 comments · Fixed by #2175
Assignees
Labels
2. developing Work in progress a11y28checked needed for a11y accessibility

Comments

@JuliaKirschenheuter
Copy link
Contributor

image

https://report.bitvtest.de/default-en/cbedb5ef-4815-4cba-a83d-995599c178c3.html#checkpoint-3dbfb7615a-v4-n4

@JuliaKirschenheuter JuliaKirschenheuter added 1. to develop Accepted and waiting to be taken care of accessibility labels Nov 29, 2023
@susnux
Copy link
Contributor

susnux commented Nov 30, 2023

Vue component issue. My opinion: we should really get rid of those button outside the modal, and if we keep them add background to it or always force dark backdrop.

@szaimen
Copy link
Contributor

szaimen commented Dec 4, 2023

I think we cannot get rid of it because the viewer uses it as well?

@szaimen szaimen self-assigned this Dec 4, 2023
@szaimen
Copy link
Contributor

szaimen commented Dec 4, 2023

So I would simply would adjust the modal in the photos app to use the close button inside the modal and/or should we force dark backdrop in the library? Also cc @nextcloud/designers

@susnux
Copy link
Contributor

susnux commented Dec 4, 2023

I would say force dark backdrop if next prev or name is set

@szaimen szaimen added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Dec 5, 2023
@szaimen szaimen transferred this issue from nextcloud/server Dec 5, 2023
@szaimen
Copy link
Contributor

szaimen commented Dec 5, 2023

I am wondering if we should by default show the name inline and give an option to show it outline?
Because https://github.com/nextcloud/photos/pull/2175/files looks a bit unintuitiv to me honestly...

WDYT @susnux @JuliaKirschenheuter @Pytal @ShGKme ?

@ShGKme
Copy link
Contributor

ShGKme commented Dec 5, 2023

I am wondering if we should by default show the name inline and give an option to show it outline? Because https://github.com/nextcloud/photos/pull/2175/files looks a bit unintuitiv to me honestly...

Yeah, I thought about it many times.

Currently, many apps manually overwrite NcModal styles, add padding, implement header and ect. Which could be broken on updates, inconsistent and not perfect for a11y.

The simplest solution could be to add new props for it, but there are so many props already.
For example, NcModal has functionality mostly needed only for viewer: switching to the next/prev slides, title above the modal.

But personally, I'd like to ideally have:

  1. A base component that provides the most general features: mounting and showing the modal, sizes ect.
  2. Simple-to-use in apps component that has a typical header, padding and ect.
  3. Separated viewer-like modal with slides and actions
  4. NcDialog for dialogs-like components: messages, prompts

So we won't have super-component with a lot of props and flags, but also have something easy to use in apps.

@susnux
Copy link
Contributor

susnux commented Dec 5, 2023

Thats why we have two components:

NcModal for everything custom like the viewer.
NcDialog for dialogs like file picker, etc. This one already provides the name as a heading inside the dialog.

@szaimen
Copy link
Contributor

szaimen commented Dec 5, 2023

So thr solution would be to migrate the photos modals to ncdialog?

@ShGKme
Copy link
Contributor

ShGKme commented Dec 5, 2023

Thats why we have two components:

NcModal for everything custom like the viewer. NcDialog for dialogs like file picker, etc. This one already provides the name as a heading inside the dialog.

Yeah, cool. I missed that dialogs are configurable enough to replace NcModals in apps in general, not only OC.dialogs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress a11y28checked needed for a11y accessibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants