-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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(v2): remove invalid label attribute of footer links #1980
Conversation
Deploy preview for docusaurus-2 ready! Built with commit eda2bc7 |
Deploy preview for docusaurus-preview ready! Built with commit eda2bc7 |
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.
It was intentional that I passed every prop through as attributes so users can add in more attributes that we don't support. That could have been a bad idea. Thoughts?
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.
Unlike #1974 this doesn't pass the rest of the props
But footer links cannot have additional attributes, unnecessarily add the rest of the props. https://v2.docusaurus.io/docs/docusaurus.config.js/#themeconfig |
I'm thinking that maybe we should pass it so that it allow overriding even the Like {
label: 'Introduction',
to: 'docs/introduction',
target: '_blank'
}, is possible |
@endiliey done ✔️, yeah, now I see, all the more reason we need to keep consistency with navlinks. |
} | ||
: { | ||
to: toUrl, | ||
})}> | ||
{item.label} | ||
{label} |
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.
Now i wonder if we should put it right before label instead ? Otherwise the props cannot override rel
and target
for href
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.
Yep, exactly, you are absolutely right, fixed!
Strange, Netlify fails, how to restart it? |
One way to restart is to push another commit 😢 |
Motivation
A continuation of #1974. This is relevant only for external links.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Before:
After (without
label
):