search

nextcloud / photos

📸 Your memories under your control
GNU Affero General Public License v3.0
571 stars 62 forks source link

[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 closed 10 months ago

JuliaKirschenheuter commented 10 months ago

image

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

susnux commented 10 months ago

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 commented 10 months ago

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

szaimen commented 10 months ago

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 commented 10 months ago

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

szaimen commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

ShGKme commented 10 months ago

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