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

refactor: use SVG icon for home breadcrumb #7183

Merged
merged 5 commits into from
Apr 20, 2022

Conversation

Dr-Electron
Copy link
Contributor

Motivation

I think this improves the looks of the breadcrumbs

old-lightnew-light
old-darknew-dark

I also gave an outlined version a quick try, but I think I still prefer the filled version 🤔
Also a wider stroke didn't really made it better for my taste 😅 . But I'm happy to change if you like it better.
new-outline-lightnew-outline-dark

Have you read the Contributing Guidelines on pull requests?

Yes

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 16, 2022
@netlify
Copy link

netlify bot commented Apr 16, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 514928d
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/625f87e7fe48870009223f09
😎 Deploy Preview https://deploy-preview-7183--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 Apr 16, 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-7183--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena changed the title Update home icon in breadcrumbs refactor: use SVG icon for home breadcrumb Apr 17, 2022
@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Apr 17, 2022
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.

Can we use the primary color so it looks slightly more like a link?

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Let's take the house icon from the Google collection? It's more compact by size, just compare:

<svg height="24px" viewBox="0 0 24 24" width="24px" fill="currentColor"><path d="M0 0h24v24H0z" fill="none"/><path d="M10 20v-6h4v6h5v-8h3L12 3 2 12h3v8z"/></svg>

@lex111
Copy link
Contributor

lex111 commented Apr 17, 2022

Can we use the primary color so it looks slightly more like a link?

It can be confusing, since primary color applies only to the active item of the breadcrumbs.

@Josh-Cena
Copy link
Collaborator

Let's take the house icon from the Google collection?

That one has sharp edges, though, while all the other icons in our design system look "rounder". (I noticed a few do have square edges but I think we should update them as well, considering our boxes are round-cornered.) At best we need to edit that icon so it has rounded corners as well.

since primary color applies only to the active item of the breadcrumbs.

Make sense

@lex111
Copy link
Contributor

lex111 commented Apr 17, 2022

They provided the rounded variant too https://fonts.google.com/icons?icon.query=home&icon.style=Rounded

@Josh-Cena
Copy link
Collaborator

Ah, that's nice!

@Josh-Cena
Copy link
Collaborator

Screen Shot 2022-04-20 at 12 15 32 PM

@lex111 FYI, there's a corner where the breadcrumb gets active state but not the link. Maybe you want to take a look?

@lex111
Copy link
Contributor

lex111 commented Apr 20, 2022

That's the way it's meant to be, or do you have something else in mind?

@Josh-Cena
Copy link
Collaborator

I think it should not be active when the cursor is not in that highlighted region. The background color should appear at the same instant as the text becomes green.

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.

Thanks, LGTM 👍

@slorber
Copy link
Collaborator

slorber commented Apr 20, 2022

The background color should appear at the same instant as the text becomes green.

Agree on that

There's something weird in Infima, because even hovering the "next" arrow, the background changes color immediately

@slorber slorber merged commit 44ebe73 into facebook:main Apr 20, 2022
@lex111
Copy link
Contributor

lex111 commented Apr 20, 2022

I think it should not be active when the cursor is not in that highlighted region. The background color should appear at the same instant as the text becomes green.

Will be fixed in facebookincubator/infima#241

@Dr-Electron
Copy link
Contributor Author

Thanks for fixing it and merging. Sorry that I didn't help. Was a busy week 😅

@Dr-Electron Dr-Electron deleted the update-home-icon branch April 21, 2022 18:46
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: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants