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(theme-classic): new 'html' type navbar item #7058

Merged
merged 5 commits into from
Apr 15, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Mar 29, 2022

Motivation

Inside a dropdown, it is sometimes useful to separate some items from others, or to group many items by adding heading text. Something like this:

image

So I propose to allow using raw html as content inside a dropdown item list.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See preview.

Related PRs

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Mar 29, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 29, 2022
@lex111 lex111 force-pushed the lex111/dropdown-menu-html branch 2 times, most recently from 112f42e to d97bb0f Compare March 29, 2022 09:30
@netlify
Copy link

netlify bot commented Mar 29, 2022

[V2]

Name Link
🔨 Latest commit b8e3e34
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6242d15f9c05880008f24a93
😎 Deploy Preview https://deploy-preview-7058--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 29, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 65
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

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

@github-actions
Copy link

github-actions bot commented Mar 29, 2022

Size Change: +774 B (0%)

Total Size: 799 kB

Filename Size Change
website/build/assets/css/styles.********.css 106 kB +101 B (0%)
website/build/assets/js/main.********.js 604 kB +486 B (0%)
website/build/index.html 38.8 kB +187 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 50 kB

compressed-size-action

@netlify
Copy link

netlify bot commented Mar 29, 2022

[V2]

Name Link
🔨 Latest commit 5efbb4e
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62594459dd962500084aee4d
😎 Deploy Preview https://deploy-preview-7058--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.

@lex111 lex111 force-pushed the lex111/dropdown-menu-html branch from d97bb0f to 9ce578e Compare March 29, 2022 09:43
@lex111 lex111 force-pushed the lex111/dropdown-menu-html branch from 3a77edc to a11b386 Compare March 29, 2022 10:05
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.

I think we should simply introduce a new type: "html" element, like { type: "html", value: "<b>Supported package managers</b>" }, so that it's more in line with the sidebar item. It would be more inconsistent with footer items, but footer items don't have multiple types and aren't polymorphic anyways.

In addition, I don't think pure HTML top-level navbar items are useless, and could potentially make things more consistent (since we only forbid nested dropdowns, and HTML elements aren't dropdowns). WDYT?

@lex111
Copy link
Contributor Author

lex111 commented Mar 29, 2022

I agree, this might useful if need to add a button or a link with an SVG icon as navbar item. But I don't know, is it necessary to keep the original class .navbar__item in this case? Should there be separate option to avoid from adding it?

@Josh-Cena
Copy link
Collaborator

The HTML sidebar items also add menu__list-item, so I think we should, for the sake of spacing consistency. Users can override them if they don't want that default style.

@Josh-Cena Josh-Cena changed the title feat(theme-classic): allow using html in dropdown items feat(theme-classic): new 'html' type navbar item Mar 29, 2022
@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Mar 30, 2022
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Nice dogfood idea

This looks a bit weird on mobile IMHO, maybe the items/separator should look more aligned?

image

…barItem.tsx

Co-authored-by: Sébastien Lorber <slorber@users.noreply.github.com>
@lex111
Copy link
Contributor Author

lex111 commented Apr 7, 2022

This looks a bit weird on mobile IMHO, maybe the items/separator should look more aligned?

Any suggestions for improvement it? I agree, it doesn't look perfect, but it's perfectly acceptable.

@slorber
Copy link
Collaborator

slorber commented Apr 7, 2022

yes, looks good enough to merge

I think it would be better if mobile would stay in between these 2 red lines

image

@Josh-Cena
Copy link
Collaborator

I agree. And in light mode the font color/size is a bit inconsistent. Would be better if we can have some special styles on mobile.

@Josh-Cena
Copy link
Collaborator

@lex111 Do you want to do some little improvements for the mobile sidebar so the styling looks more consistent, or do you think this is good enough to merge?

@lex111
Copy link
Contributor Author

lex111 commented Apr 15, 2022

No, I think the current styling is good enough.
I can see that the color of heading is other, but if we want to fix it, we have to do it in Infima, but I would prefer not to do that.

@Josh-Cena
Copy link
Collaborator

I see what you mean, all good then!

@Josh-Cena Josh-Cena merged commit 84d04ed into main Apr 15, 2022
@Josh-Cena Josh-Cena deleted the lex111/dropdown-menu-html branch April 15, 2022 10:58
@Josh-Cena Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Apr 15, 2022
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