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

Adjust Nav Styling #1525

Merged
merged 8 commits into from Feb 25, 2022
Merged

Adjust Nav Styling #1525

merged 8 commits into from Feb 25, 2022

Conversation

joshri
Copy link
Contributor

@joshri joshri commented Feb 24, 2022

Closes: #1459

image

All you have to do to make a Nav Item gray is add the key/value {sub: true} to the navItems object array.

Also did some fixes for the logo, background, and border radius on main containers.

@joshri joshri added the area/ui Issues that require front-end work label Feb 24, 2022
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

This doesn't look like the designs. We probably need to change the font of the sub menu item and make sure that menu items are grouped together.

ui/components/Logo.tsx Show resolved Hide resolved
@@ -17,7 +17,7 @@ function SourcesList({ className }: Props) {
loading={isLoading}
className={className}
>
<SourcesTable sources={sources} />
<SourcesTable sources={sources || []} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a leaky abstraction. Can we make <SourcesTable /> handle undefined instead?

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'm going to remove this and handle it when I do the filtering. It has nothing to do with the Nav I was just checking out the errors

@joshri
Copy link
Contributor Author

joshri commented Feb 24, 2022

@jpellizzari good call out on the grouping together - I missed that. Sub menu item font I think needs to go from 600 - 400 font weight based on figma

@joshri
Copy link
Contributor Author

joshri commented Feb 25, 2022

image

FIXED

Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

Nice work on the sub items pattern. One nit about the distance, but assuming that matches the Figma drawing, LGTM.

/>
{_.map(n.subItems, (sub) => {
return (
<StyleLinkTab
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at Figma, sub items should be 12px below the main one. This looks a like a little bigger gap.

@joshri
Copy link
Contributor Author

joshri commented Feb 25, 2022

FIXED AGAIN
image

@joshri joshri merged commit 5747caf into v2 Feb 25, 2022
@joshri joshri deleted the 1459-nav branch February 25, 2022 19:49
jpellizzari added a commit that referenced this pull request Mar 3, 2022
* fix logo text and bg, initial tabs work wrestling with mui

* nav styles

* snapshots updated

* Set font-weight on .sub-items

* nav spacing and font weight

* remove dupe

* brought indicator back

* brought back proper spacing with new structure for indicator

Co-authored-by: Jordan Pellizzari <jordan@weave.works>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui Issues that require front-end work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants