nextcloud-vue icon indicating copy to clipboard operation
nextcloud-vue copied to clipboard

fix: regression in list item's extra slot

Open hamza221 opened this issue 1 year ago • 14 comments

☑️ Resolves

  • Fix https://github.com/nextcloud/mail/issues/9334

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🚧 Tasks

  • [ ] ...

🏁 Checklist

  • [ ] ⛑️ Tests are included or are not applicable
  • [ ] 📘 Component documentation has been extended, updated or is not applicable
  • [ ] 3️⃣ Backport to next requested with a Vue 3 upgrade

hamza221 avatar Feb 15 '24 10:02 hamza221

cc @GretaD @susnux @ShGKme

hamza221 avatar Feb 15 '24 10:02 hamza221

The "after" image also looks broken, for the NcListItem I would only expect two lines of text working and the tags to be shown on the very right.

susnux avatar Feb 15 '24 12:02 susnux

The "after" image also looks broken,

Yes the after is still a bit broken, the tags should be aligned with the lines This is how it looks on nc.cloud image

for the NcListItem I would only expect two lines of text working and the tags to be shown on the very right.

Isn't the extra slot supposed to show elements below the lines

hamza221 avatar Feb 15 '24 12:02 hamza221

The "after" image also looks broken,

Yes the after is still a bit broken, the tags should be aligned with the lines This is how it looks on nc.cloud

this is fixed on mail side, i already pushed a pr for that https://github.com/nextcloud/mail/pull/9336

GretaD avatar Feb 15 '24 13:02 GretaD

Was there a design review? It seems this component gets extended to an all-in-one-solution. Especially that there is now a 3rd and 4th line introduced, which might be ok but we need to make sure not to break anything (e.g. a11y where you need to make sure to NOT include interactive elements ).

susnux avatar Feb 16 '24 18:02 susnux

Makes more sense now thank you for the explanation, I'll try to work on a different solution

hamza221 avatar Feb 16 '24 19:02 hamza221

I mean if it fits in the component fine, but we can not have nested interactive elements. This only works with some specific focus handling (e.g. how menu bars work, you can tab the first element but next tab will be outside the menu bar, you then need to use arrows to change focus).

susnux avatar Feb 18 '24 16:02 susnux

Hello @susnux, this is still an issue on mail, i would get @jancborchardt @marcoambrosini @nimishavijay involved, what would be a good compromise :)

If it was up to me, I would keep it on the right side, with the argument, that its like this since couple of months and nobody complained about the space, which means that either people are fine with it, or its not used at all :), now that we have different layouts, people can always choose a layout where the list is wide. But, of course, i would be fine to work on another solution.

GretaD avatar Jun 05 '24 11:06 GretaD

An interactive element (the a) must not contain any other interactive element (the NcActions).

Are you sure that this is the case @susnux ? afaik an <a> cannot contain another <a> but it can contain buttons

e.g. most cards in card layouts Screenshot 2024-06-05 at 17 34 58

marcoambrosini avatar Jun 05 '24 15:06 marcoambrosini

e.g. most cards in card layouts

In valid implementation, they are not wrapped in <a>. They have a link inside and either use custom onclick (like we do in Files) or stretch the link in a pseudo element (like we do in Apps list).

But in both HTML validity and accessibility having nested interactive elements is not valid.

ShGKme avatar Jun 05 '24 17:06 ShGKme

  • See: https://github.com/nextcloud-libraries/nextcloud-vue/pull/5198

ShGKme avatar Jun 05 '24 17:06 ShGKme

thank you for all the feedback,

@marcoambrosini how should we proceed then?

GretaD avatar Jun 18 '24 11:06 GretaD

I donn't know which would be the better approach. cc @ShGKme and @susnux

marcoambrosini avatar Jun 18 '24 11:06 marcoambrosini

what we're trying to accomplish is just a third line for 'non interactive- non clickable "tags" ' would that be acceptable ? So move the slot outside the NcActions rename it (thirdline? ) ?

hamza221 avatar Jul 22 '24 14:07 hamza221