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

[docs] Fix missing href for AppDrawerNavItems #27936

Merged
merged 2 commits into from Aug 24, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Aug 24, 2021

The nav items didn't include the common affordances for links (e.g. open new tab, copy link adress etc).

Before: https://6124b8cbdf676e0008cc3e43--material-ui.netlify.app/components/box/
After: https://deploy-preview-27936--material-ui.netlify.app/components/box/

Bisecting lead to 49f21f8 which makes sense since the child element of next/link no longer had the a type and therefore needed an explicit passHref.

It's unclear why passHref was a supported prop. It wouldn't be useful since the child type is controlled by the implementation not its usage. But we may have destructured it in NextLinkComposed because its callsites didn't properly filter passHref. This is a responsibility of the callsite of NextLinkComposed. Good thing is that passHref leaking to an a would trigger React warnings so we'll see if this is/was ever an issue.

to: NextLinkProps['href'];
linkAs?: NextLinkProps['as'];
href?: NextLinkProps['href'];
}

const Anchor = styled('a')({ cursor: 'pointer' });

export const NextLinkComposed = React.forwardRef<HTMLAnchorElement, NextLinkComposedProps>(
const NextLinkComposed = React.forwardRef<HTMLAnchorElement, NextLinkComposedProps>(
Copy link
Member Author

Choose a reason for hiding this comment

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

Never used so don't export it.

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 24, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 2fec1ef

@eps1lon eps1lon added bug 🐛 Something doesn't work docs Improvements or additions to the documentation labels Aug 24, 2021
@eps1lon eps1lon marked this pull request as ready for review August 24, 2021 10:42
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍

@eps1lon eps1lon merged commit 6ea1867 into mui:next Aug 24, 2021
@eps1lon eps1lon deleted the docs/fix/anchor-href branch August 24, 2021 11:03
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 24, 2021

Thoughts:

  1. @siriwatknp Do we really need const Anchor = styled('a')({ cursor: 'pointer' });? First, this cursor pointer is suspicious/redundant. Aren't the user agents already adding it for valid links? Second, the responsibility of the NextLinkComposed is to implement the navigation behavior, not the styles, adding styled() for the sx prop seems to leak the intended abstraction, I think that it should only be in Link.
  2. If the concerns in 1. are not valid, should we update the /examples too with this revisited implementation of the Next.js' Link component? e.g. for next-typecript. So far we have used the docs to dog food this component and shared it with the community as a code snippet.
    It also makes me think about https://twitter.com/SquishyDough/status/1390326410972176391, getting this Link component right might be tricky for the community.
  3. "It's unclear why passHref was a supported prop." Agree 👍 the prop doesn't seem relevant in the context of NextLinkComposed.

oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Sep 5, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Sep 5, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants