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

fix(NcListItem): Remove actions from within a which caused invalid HTML output #5198

Merged
merged 2 commits into from Feb 2, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 31, 2024

☑️ Resolves

It is not valid to put button inside a thus we need to refactor the NcListItem:

  1. Unify the actions location within the source to be outside the main wrapper
  2. Replace wrapper a with div
  3. Make only the main content an a , this is required so users can right click + copy link or middle click -> native browser behavior.
  4. Not related but fix focus-visible to have consistent style across all components.

Hint: Hide white space changes when review

🖼️ Screenshots

Could not spot visual changes except from focus visible.

a.mp4

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@susnux susnux added bug Something isn't working 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: list-item Related to the list-item component labels Jan 31, 2024
@susnux susnux changed the title Fix/nc list item fix(NcListItem): Remove actions from within a which caused invalid HTML output Jan 31, 2024
src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (6f1a4c9) 39.34% compared to head (26f3553) 39.30%.
Report is 18 commits behind head on master.

❗ Current head 26f3553 differs from pull request most recent head 0886575. Consider uploading reports for the commit 0886575 to get more accurate results

Files Patch % Lines
src/components/NcListItem/NcListItem.vue 3.12% 30 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5198      +/-   ##
==========================================
- Coverage   39.34%   39.30%   -0.05%     
==========================================
  Files         139      139              
  Lines        3688     3692       +4     
  Branches      810      811       +1     
==========================================
  Hits         1451     1451              
- Misses       2021     2025       +4     
  Partials      216      216              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@GretaD
Copy link
Contributor

GretaD commented Feb 2, 2024

Hello @susnux please keep in mind that we are changing the design for mail. I will wait for this to be finished before a continue here: #5209
But just so you know, if you can make my life easier :)

@susnux susnux force-pushed the fix/nc-list-item branch 3 times, most recently from 35eafd2 to 6e45c2e Compare February 2, 2024 14:02
@susnux
Copy link
Contributor Author

susnux commented Feb 2, 2024

Ready to review I would say 🚀

vokoscreenNG-2024-02-02_15-02-36.mp4

@ShGKme
Copy link
Contributor

ShGKme commented Feb 2, 2024

Compact is a little bit broken, the height is changed on hover.

compact

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine now with both Tab navigation and the screen reader quick navigation. The only issue I've found is the height on hover in compact mode

src/components/NcListItem/NcListItem.vue Show resolved Hide resolved
@susnux
Copy link
Contributor Author

susnux commented Feb 2, 2024

Compact is a little bit broken, the height is changed on hover.

@ShGKme I fixed this now:

vokoscreenNG-2024-02-02_16-03-38.mp4

@susnux susnux requested a review from ShGKme February 2, 2024 15:04
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems to work fine now, thanks!

src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
…e to use of button within anchor

Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added this to the 8.6.2 milestone Feb 2, 2024
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@susnux susnux merged commit 1733f8d into master Feb 2, 2024
18 checks passed
@susnux susnux deleted the fix/nc-list-item branch February 2, 2024 16:44
ShGKme pushed a commit that referenced this pull request Feb 5, 2024
fix(NcListItem): Remove actions from within `a` which caused invalid HTML output
@ShGKme ShGKme mentioned this pull request Feb 7, 2024
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 accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: list-item Related to the list-item component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV]: Versions: The element button must not appear as a descendant of the a element.
7 participants