-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix: regression in list item's extra slot #5250
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Hamza Mahjoubi <[email protected]>
The "after" image also looks broken, for the |
this is fixed on mail side, i already pushed a pr for that nextcloud/mail#9336 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes reverting a11y fixes and are not valid -> must not be done. You have to find a solution with the actions outside the anchor element
@@ -394,21 +394,20 @@ | |||
</div> | |||
</div> | |||
</div> | |||
<!-- Actions --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must not be done! An interactive element (the a
) must not contain any other interactive element (the NcActions
).
We changed this just a couple of weeks ago.
Was there a design review? It seems this component gets extended to an all-in-one-solution. |
Makes more sense now thank you for the explanation, I'll try to work on a different solution |
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). |
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. |
Are you sure that this is the case @susnux ? afaik an |
In valid implementation, they are not wrapped in But in both HTML validity and accessibility having nested interactive elements is not valid. |
thank you for all the feedback, @marcoambrosini how should we proceed then? |
what we're trying to accomplish is just a third line for 'non interactive- non clickable "tags" ' would that be acceptable ? |
☑️ Resolves
🖼️ Screenshots
🚧 Tasks
🏁 Checklist
next
requested with a Vue 3 upgrade