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

Update Nav design #2514

Merged
merged 12 commits into from
Jun 7, 2021
Merged

Update Nav design #2514

merged 12 commits into from
Jun 7, 2021

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented May 30, 2021

  • Updated nav design based off Figma design in Setup Universal Header/Footers #2440
  • I also edited the mobile menu since the tabbing wasn't working correctly.
  • There is some crowding in between screen size changes. I lowered the margins in some places to keep the crowding as small as possible but if you have any suggestions to improve it, please let me know!
  • Since there were no focus/active styles on Figma, I just went with I thought matched the site styles originally but let me know if any the styles should be changed!
    Fixes Setup Universal Header/Footers #2440

@shcheklein shcheklein temporarily deployed to dvc-org-update-nav-cggje4ymxlr May 30, 2021 22:59 Inactive
@julieg18
Copy link
Contributor Author

julieg18 commented May 30, 2021

Looks like I'll need to do some refactoring with the nav and the header popup JSX to correct the code climate errors. I'll get on it as soon as I can!

@media (--sm-scr) {
margin-left: 4px;

&:nth-child(5),
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 used CSS to take off the nav items, but we could also use JS to check for screen overflow and take off the items with JS.

@julieg18
Copy link
Contributor Author

Should we close the popups if a user clicks on one of the popup links? 🤔

@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr May 31, 2021 12:19 Inactive
* add SocialIcons component
* reduce popup code
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr May 31, 2021 14:18 Inactive
* place nav popup handler code in Nav/Popup hook, useNavPopups
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr May 31, 2021 16:15 Inactive
* increase popup font sizes
* take off letter-spacing in all elements
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr June 1, 2021 20:37 Inactive
@arcticbear
Copy link

Should we close the popups if a user clicks on one of the popup links? 🤔

Good question. For example, in the case when a user opened a popup and with Cmd/Ctrl + clicking on links to open them in the separate tabs it would be useful to keep the popup opened after every click. But in the case of single clicks, we should close the popup, especially when we open a new page in the current tab.

@rogermparent
Copy link
Contributor

But in the case of single clicks, we should close the popup, especially when we open a new page in the current tab.

On top of that, the Community menu is all links to different headers within the same page. I think we'll want to dismiss the menu on click there as well, since I don't see users wanting to go through multiple headers in one opening of the menu.

@shcheklein
Copy link
Member

shcheklein commented Jun 1, 2021

Is it only me, or it looks too heavy with this font size/weight and so many items? (it take almost of all my screen and fonts size is close to some headers on the page?)

@rogermparent
Copy link
Contributor

rogermparent commented Jun 1, 2021

Is it only me, or it looks too heavy with this font size/weight and so many items? (it take almost of all my screen and fonts size is close to some headers on the page?)

Agreed, this is particularly noticeable at >1000px width, but even the breakpoint below seems a tad heavy.

@julieg18
Copy link
Contributor Author

julieg18 commented Jun 1, 2021

Is it only me, or it looks too heavy with this font size/weight and so many items? (it take almost of all my screen and fonts size is close to some headers on the page?)

Also agreed! What if we made another drop-down that held some of the other nav links on smaller screen sizes?🤔

Copy link

@arcticbear arcticbear left a comment

Choose a reason for hiding this comment

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

Also, the links Community and Meet the community are similar I guess, they lead to the sections which very close to each other on the top of the same page. What do you think if we hide the second one and rename the first to Meet the community?

src/components/LayoutHeader/Nav/Popup/styles.module.css Outdated Show resolved Hide resolved
src/components/LayoutHeader/Nav/Popup/styles.module.css Outdated Show resolved Hide resolved
* increase line-height in .description
* edit community links
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr June 2, 2021 12:11 Inactive
* move nav list into separate component
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr June 2, 2021 12:52 Inactive
* move useNavPopups hook to Popup/util.tsx
* split useNavPopups into multiple functions
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr June 2, 2021 15:04 Inactive
julieg18 added 2 commits June 2, 2021 15:35
* move Popup event listeners to LinkItems, lowering code complexity
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr June 2, 2021 20:44 Inactive
* Take external link icon off popup link if link is not external
@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr June 2, 2021 20:52 Inactive
@julieg18
Copy link
Contributor Author

julieg18 commented Jun 2, 2021

Ok, I completed all of @arcticbear's comments and update the nav popups to close on link click!

@julieg18 julieg18 requested a review from arcticbear June 2, 2021 21:18
@rogermparent
Copy link
Contributor

rogermparent commented Jun 3, 2021

I'd say the font sizes could be a little smaller as well, and I'm not too big a fan of having different sets of items available at different breakpoints- the current dvc.org displays either all items on the header or all items in the burger menu, which I think is a better way to handle display sizes. Now that I look at it, we'll also need to add "Other Tools" to the burger menu.

Alternatively, we could have a burger menu that handles all items not displayed on the header, likely by always having all items and only displaying the menu when any item is hidden.

If we decrease the font size, we can and should keep the tap targets 48px with padding or some other method of sizing, even if we must reduce the padding/margin of parent containers such that more of the total header area is tappable.

@shcheklein
Copy link
Member

@arcticbear could you please find some time to review this? :)

@julieg18
Copy link
Contributor Author

julieg18 commented Jun 4, 2021

Just noticed that some of the footer svgs disappear when the popups disappear:

image
image

UPDATE
I turned the popup inline svgs to background images for now.

@julieg18 julieg18 temporarily deployed to dvc-org-update-nav-cggje4ymxlr June 4, 2021 16:24 Inactive
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Looks good enough to me as a first iteration!

@julieg18 julieg18 merged commit f032de7 into master Jun 7, 2021
@julieg18 julieg18 mentioned this pull request Jun 7, 2021
2 tasks
@shcheklein shcheklein deleted the update-nav branch July 23, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup Universal Header/Footers
4 participants