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: header component #83 #102

Merged
merged 48 commits into from
May 18, 2020
Merged

Conversation

eamahanna
Copy link
Contributor

@eamahanna eamahanna commented Apr 23, 2020

PR Description

This PR creates components which are necessary to create the uswds basic header described here.

Created Components:

  • Header
  • NavList
  • Title (Already okay'd in previous pr)
  • MegaMenu
  • PrimaryNav
  • Menu
  • NavButton
  • ExtendedNav

Each of the components has associated stories and tests.

For Reviewers

This is a huge PR......my bad. Next time I will try and break up the changes better. The title component is good to go already. Please let me know if there are any organization changes. I broke up the components the best I could, but having another set of eyes on everything is good! Thank you in advance!

@eamahanna eamahanna added the status: wip Work in progress - not ready for code review label Apr 23, 2020
@haworku
Copy link
Contributor

haworku commented Apr 23, 2020

Hey @eamahanna realized I have something similar to the navigation work you are taking on in the Footer work. I'd love to pair on with you!

@suzubara
Copy link
Contributor

Had some small nitpick comments after reading tests and comparing w/ the uswds markup.
Also question - theExtendedHeader and Header look the same except for a single class. To me this looks like a single component <Header/> with a prop extended. Am I missing something?

This is something I discussed with @suzubara. She suggested making to different components for each to keep them a little cleaner. I am happy to make the extended prop though. @suzubara thoughts?

Hm I thought there was some difference in markup between the two as well? If it's just a matter of an additional CSS class then an extended prop sounds fine..

Copy link
Contributor

@suzubara suzubara 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 regroup on organizing the various header/footer/nav subcomponents!

src/components/header/Header/Header.tsx Outdated Show resolved Hide resolved
src/components/header/MegaMenu/MegaMenu.tsx Outdated Show resolved Hide resolved
src/components/header/MegaMenu/MegaMenu.tsx Show resolved Hide resolved
src/components/header/Menu/Menu.tsx Outdated Show resolved Hide resolved
src/components/header/NavButton/NavButton.tsx Outdated Show resolved Hide resolved
src/components/header/PrimaryNav/PrimaryNav.tsx Outdated Show resolved Hide resolved
src/components/header/PrimaryNav/PrimaryNav.tsx Outdated Show resolved Hide resolved
src/components/header/PrimaryNav/PrimaryNav.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@suzubara suzubara left a comment

Choose a reason for hiding this comment

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

I left a lot of more nitpicky comments now that I've had a chance to compare this markup with the USWDS markup. let me know if there's anything I can help with or needs clarification!

Additionally, nav items should be able to be indicated as "current" (so they can apply the usa-current class). I think the class may be applied to the simple links directly in the stories, but menu items & dropdown buttons may need to accept a prop like isCurrent in order to apply the class

src/components/header/Header/Header.stories.tsx Outdated Show resolved Hide resolved
src/components/header/MegaMenu/MegaMenu.tsx Outdated Show resolved Hide resolved
src/components/header/NavButton/NavButton.tsx Outdated Show resolved Hide resolved
src/components/header/NavDropDown/NavDropDown.tsx Outdated Show resolved Hide resolved
src/components/header/NavList/NavList.tsx Outdated Show resolved Hide resolved
src/components/header/NavList/NavList.tsx Show resolved Hide resolved
@haworku
Copy link
Contributor

haworku commented May 12, 2020

@suzubara Thanks for bringing up the current thing for the nav I've been wondering if this should be a prop for the header /footer work. I'll try to implement in the footer but I may have more questions about this.

@suzubara
Copy link
Contributor

@suzubara Thanks for bringing up the current thing for the nav I've been wondering if this should be a prop for the header /footer work. I'll try to implement in the footer but I may have more questions about this.

👍 ok, I'm not sure if it's relevant for the footer since I think it's less common for footer links to show a "current" class (as opposed to the header nav)

@eamahanna eamahanna requested a review from suzubara May 14, 2020 16:29
Copy link
Contributor

@haworku haworku left a comment

Choose a reason for hiding this comment

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

I see nothing major blocking. I'm going to approve since my comments are just cleanup, trust @eamahanna to integrate what makes sense.

Copy link
Contributor

@suzubara suzubara left a comment

Choose a reason for hiding this comment

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

Great work, you took on a whole lot with this component! all of my comments are suggestions, I'm approving and leaving it up to you if you want to incorporate them :)

src/components/header/ExtendedNav/ExtendedNav.test.tsx Outdated Show resolved Hide resolved
src/components/header/ExtendedNav/ExtendedNav.tsx Outdated Show resolved Hide resolved
src/components/header/ExtendedNav/ExtendedNav.tsx Outdated Show resolved Hide resolved
src/components/header/NavList/NavList.tsx Outdated Show resolved Hide resolved
src/components/header/PrimaryNav/PrimaryNav.tsx Outdated Show resolved Hide resolved
@eamahanna eamahanna changed the title Create Basic header component #83 Feat: create basic header component #83 May 18, 2020
@eamahanna eamahanna changed the title Feat: create basic header component #83 feat: create basic header component #83 May 18, 2020
@eamahanna eamahanna changed the title feat: create basic header component #83 feat: header component #83 May 18, 2020
@eamahanna eamahanna merged commit 4281b11 into develop May 18, 2020
@eamahanna eamahanna deleted the em-create-header-component-#83 branch May 18, 2020 20:40
@suzubara suzubara mentioned this pull request May 19, 2020
@haworku haworku mentioned this pull request Jun 29, 2020
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.

3 participants