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

feat: allow using pure HTML as label in navbar links #7079

Merged
merged 2 commits into from
Apr 7, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Mar 31, 2022

Motivation

Although a new html type for navbar items is coming soon, it does not solve the issue of having to use HTML as label for a navbar link. It can be useful to style text content of a link or add an SVG icon inside it without using custom CSS.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See tests as examples. Is it worth to use the option on the website as applying of dogfooding approach?

Related PRs

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Mar 31, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 31, 2022
@netlify
Copy link

netlify bot commented Mar 31, 2022

[V2]

Name Link
🔨 Latest commit fb06f09
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/624eba5c1a546e0009062b4a
😎 Deploy Preview https://deploy-preview-7079--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Mar 31, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 41
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-7079--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Mar 31, 2022

Size Change: +217 B (0%)

Total Size: 804 kB

Filename Size Change
website/build/assets/css/styles.********.css 105 kB +206 B (0%)
website/build/assets/js/main.********.js 610 kB -46 B (0%)
website/build/index.html 38.6 kB +57 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB

compressed-size-action

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Need to mark this as translatable. I can't remember allowing translation of any HTML label though, might introduce injection if translation is outsourced?

@lex111
Copy link
Contributor Author

lex111 commented Apr 1, 2022

It doesn't seem that easy. Currently we have translatable html-content fields?

might introduce injection if translation is outsourced?

I don't get what you mean

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Apr 1, 2022

If translation is done through Crowdin and synced during CI, malicious actors can easily inline JS or markup in the code translation, and lead to XSS. This is essentially calling dangerouslySetInnerHtml on external code. I think we don't have any translatable HTML label for that cause (footer HTML labels and announcement text are both untranslatable, see also #4542), so sure, not doing it seems like a better move at this point

@slorber
Copy link
Collaborator

slorber commented Apr 6, 2022

Looks fine to me 👍

Agree there's a security risk in translating this HTML fragment.

What about merging this as is? It's a niche use-case and translating HTML fragments is probably not the best idea anyway.

There are workarounds like build the site with multiple configs and env variables etc

@lex111
Copy link
Contributor Author

lex111 commented Apr 6, 2022

Sounds good, although in the future we should probably somehow find a way to provide translations for such HTML-like messages.

Comment on lines 60 to 73
? {dangerouslySetInnerHTML: {__html: html}}
: {
children: (
<>
{label}
{isExternalLink && (
<IconExternalLink
{...(isDropdownLink && {width: 12, height: 12})}
/>
)}
</>
),
})}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-blocking but this looks a bit messy & too deeply nested for me. If you can find a better way to structure this (I don't have much idea about how) before merge feel free to refactor it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have that feeling too, but can't think of a better solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

refactored a bit 👍

@slorber slorber merged commit bfbc78e into main Apr 7, 2022
@slorber slorber deleted the lex111/navbar-item-html branch April 7, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants