Skip to content

MudChip: Use anchor or button tag instead of div when appropriate#10488

Merged
danielchalmers merged 23 commits intoMudBlazor:devfrom
danielchalmers:chip-href
Jan 9, 2025
Merged

MudChip: Use anchor or button tag instead of div when appropriate#10488
danielchalmers merged 23 commits intoMudBlazor:devfrom
danielchalmers:chip-href

Conversation

@danielchalmers
Copy link
Member

@danielchalmers danielchalmers commented Dec 19, 2024

Description

The MudChip now renders a semantic anchor tag in place if the div if an href is specified. This improves accessibility, including allowing the user to hover it to see the link it will direct them to, middle click to open in a new tab, or drag to another place.

Additionally, Rel was added (as seen in MudButton).

It can now be a true anchor and so no longer acts like a button. Breaking changes:

How Has This Been Tested?

unit, visually

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library PR: needs review labels Dec 19, 2024
@codecov
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.54%. Comparing base (152adcd) to head (60ffd7d).
Report is 40 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10488      +/-   ##
==========================================
- Coverage   91.58%   91.54%   -0.05%     
==========================================
  Files         426      425       -1     
  Lines       13278    13319      +41     
  Branches     2550     2564      +14     
==========================================
+ Hits        12161    12193      +32     
- Misses        546      551       +5     
- Partials      571      575       +4     

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

class="@Classname"
style="@Style"
@onclick="this.AsNonRenderingEventHandler<MouseEventArgs>(OnClickAsync)">
<MudElement HtmlTag="@GetHtmlTag()"
Copy link
Member

Choose a reason for hiding this comment

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

Won't have time to fully check the PR until later today but we recently ran into a problem at work where we wanted to have interactive content within a tags, only to find out that this violates the HTML spec (and we couldn't get events to only propagate to the children, it always triggered the anchor click):

image

I think we would have to either disable the IsClosable render path when an anchor tag is used, or move the IsClosable render path out of the anchor tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be better if the close button was overlayed on top instead of nested inside?

Copy link
Member

Choose a reason for hiding this comment

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

The scenario at my work place was to have a clickable card as well as action buttons on the right side of it.

We had essentially this setup:

<ClickableCard> @* translates into an anchor tag *@
  <MudGrid>
    <SomeVisualComponents />
    <SomeActionButtons />
  </MudGrid>
</ClickableCard>

Since we couldn't get this to work because of the anchor tag issue we are going to do something like this (not yet implemented):

<ClickableCard> @* no anchor tag *@
  <CardContent>
    <SomeVisualComponents />
  </CardContent>
  <ActionAreaContent>
    <SomeActionButtons />
  </ActionAreaContent>
</ClickableCard>

The ClickableCard handles hover state/styling, the CardContent is the anchor tag, the ActionAreaContent is the extra area that is not part of the anchor tag. The downside here is that you give up the space taken by the buttons as clickable area.

The alternative would have been to, like you said, overlay the button on top of the clickable area. For us that was too annoying to handle, especially with multiple buttons and potentially other content within that second area.

FYI I just tried with MUI and they don't handle that case either. Their link Chips are also anchor tags, but if you make the chip "deletable" they add an SVG with an onclick handler (so technically not HTML spec violation), but of course when you click on the SVG the anchor tag triggers.

@danielchalmers danielchalmers changed the title Chip: Use anchor tag if href is specified Chip: Use anchor or button tag if appropriate Dec 20, 2024
@danielchalmers danielchalmers changed the title Chip: Use anchor or button tag if appropriate Chip: Use anchor or button tag instead of div when appropriate Dec 20, 2024
Copy link
Member

@igotinfected igotinfected left a comment

Choose a reason for hiding this comment

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

LGTM other than the anchor/interactive children issue!

@henon henon changed the title Chip: Use anchor or button tag instead of div when appropriate MudChip: Use anchor or button tag instead of div when appropriate Dec 20, 2024
@danielchalmers danielchalmers added the breaking change This change will require consumer code updates label Jan 3, 2025
@danielchalmers danielchalmers marked this pull request as ready for review January 3, 2025 14:40
@danielchalmers
Copy link
Member Author

danielchalmers commented Jan 3, 2025

I've disabled the Enter key intercept, OnClick event, and Close button for anchor chips to not interfere with the browser's default behavior. Does anyone agree/disagree? May have to cut my losses and close the PR because I couldn't find another way.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 3, 2025

@danielchalmers
Copy link
Member Author

Enter key intercept, OnClick event, and Close button are all removed for anchor chips to not interfere with the browser's default behavior. Is this reasonable or should I close the PR? @igotinfected @ScarletKuro @henon @mckaragoz

@henon
Copy link
Contributor

henon commented Jan 7, 2025

I think it is reasonable. You agree @igotinfected ?

@igotinfected
Copy link
Member

Yup makes sense to me!

@danielchalmers
Copy link
Member Author

Added to migration guide:

The MudChip now renders a semantic anchor tag in place if the div if an href is specified. It can now be a true anchor and so no longer acts like a button. The browser now handles the click (and Enter key) so OnClick is disabled and the close button is no longer shown (if applicable). This improves accessibility, including allowing the user to hover it to see the link it will direct them to, middle click to open in a new tab, or drag to another place.

@danielchalmers danielchalmers merged commit a45d81d into MudBlazor:dev Jan 9, 2025
6 checks passed
@danielchalmers danielchalmers deleted the chip-href branch January 9, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants