feat: ENG-283 Nav update #638
Conversation
| @@ -53,3 +53,15 @@ To publish | |||
| ``` | |||
| $ yarn publish:npm | |||
| ``` | |||
|
|
|||
| ### Testing | |||
There was a problem hiding this comment.
Just adding some missing docs while I'm here
| transition: opacity 0.2s; | ||
| position: relative; | ||
|
|
||
| ${({ isHeader }) => isHeader && css` |
There was a problem hiding this comment.
Only setting and passing this in the new Header2024, so the pre-existing Header Classic is barely affected
| // First level of the navigation (ul tag): Parent | ||
| <NavMenu role="menubar"> | ||
| {/* | ||
| **************************** |
There was a problem hiding this comment.
Heavy-handed comments for the sake of sanity; both while developing and reading
| Header2024.propTypes = { | ||
| // Check data structure example in src/components/moleculecules/header/data/data | ||
| navItems: PropTypes.objectOf(PropTypes.shape), | ||
| // NB: metaIcons no longer include the Donate button: | ||
| metaIcons: PropTypes.node.isRequired, | ||
| // ... and is supplied separately to allow more render control: | ||
| donateButton: PropTypes.node, | ||
| campaign: PropTypes.string | ||
| }; | ||
|
|
||
| export default Header2024; |
There was a problem hiding this comment.
Possibly define the shape?
navItems: PropTypes.shape({
menuGroups: PropTypes.arrayOf(
PropTypes.shape({
id: PropTypes.string.isRequired,
links: PropTypes.arrayOf(
PropTypes.shape({
title: PropTypes.string.isRequired,
url: PropTypes.string,
})
),
})
),
}),
Might be handy if we ever want to swap to Typescript, as would be one less yelling error?
There was a problem hiding this comment.
Good shout for sure; I'm not 100% sure if this CMS-defined shape is consistent to this exact pattern (as I think menuGroups accept a few different content types) but this is a big improvement, thanks man; can panelbeat issues as they crop up in-situ.
Fair comment though, will give it a go! |
PR description
What is it doing?
Adds the brand new Nav2024 (name may be changed shortly) organism and associated components to provide extended functionality to the preexisting one, namely the ability to render nav menu content over a certain character threshold.
Small design tweaks to follow from Curtis, but good to get some other eyes on this, although we won't be going live with this before the new year.
Why is this required?
Less limits on what we can display in the header nav
link to Jira ticket:
https://comicrelief.atlassian.net/browse/ENG-2834
Quick Checklist:
My PR title follows the Conventional Commit spec.
I have filled out the PR description as per the template above.
I have added tests to cover new or changed behaviour.
I have updated any relevant documentation.
Important! - lastly, make sure to squash merge...