[Fairground 🎡] Add expanded nav components #11572
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: 0 B Total Size: 866 kB ℹ️ View Unchanged
|
frederickobrien
force-pushed
the
fairground/veggie-burger
branch
from
f99baba
to
9c732e3
Compare
frederickobrien
changed the title
frederickobrien
force-pushed
the
fairground/veggie-burger
branch
4 times, most recently
from
79f16ba
to
c58aa33
Compare
frederickobrien
force-pushed
the
fairground/veggie-burger
branch
from
c58aa33
to
ac8da14
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
cemms1
changed the base branch from
main
to
fairground/titlepiece-use-nav-object
cemms1
force-pushed
the
fairground/titlepiece-use-nav-object
branch
from
8fd783b
to
095a8a4
Compare
cemms1
force-pushed
the
fairground/veggie-burger
branch
from
ac8da14
to
6812e2d
Compare
cemms1
reviewed
Jul 1, 2024
Comment on lines
+15
to
+42
| // const wrapperMainMenuStyles = css` | ||
| // background-color: rgba(0, 0, 0, 0.5); | ||
| // ${getZIndex('expanded-veggie-menu-wrapper')} | ||
| // left: 0; | ||
| // top: 0; | ||
| // /* | ||
| // IMPORTANT NOTE: | ||
| // we need to specify the adjacent path to the a (current) tag | ||
| // to apply styles to the nested tabs due to the fact we use ~ | ||
| // to support NoJS | ||
| // */ | ||
| // /* stylelint-disable-next-line selector-type-no-unknown */ | ||
| // ${`#${navInputCheckboxId}`}:checked ~ div & { | ||
| // ${from.desktop} { | ||
| // display: block; | ||
| // overflow: visible; | ||
| // } | ||
| // } | ||
|
|
||
| // /* refer to comment above */ | ||
| // /* stylelint-disable */ | ||
| // ${`#${navInputCheckboxId}`}:checked ~ div & { | ||
| // ${until.desktop} { | ||
| // transform: translateX( | ||
| // 0% | ||
| // ); /* when translateX is set to 0% it reapears on the screen */ | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Nitpick/one for next time: it makes it slightly easier to review if unused code/commented out code is removed from the PR changes 👍
dotcom-rendering/src/components/Masthead/Titlepiece/ExpandedNav/ExpandedNav.tsx
Show resolved
Hide resolved
Comment on lines
+116
to
+120
| {/* <div | ||
| id="expanded-menu" | ||
| data-testid="expanded-menu" | ||
| css={wrapperMainMenuStyles} | ||
| > */} |
There was a problem hiding this comment.
One for next time - If leaving code commented out when putting up for review, it's helpful to leave another comment to say why it's there
Comment on lines
42
to
47
| /** | ||
| * Typography preset styles should not be overridden. | ||
| * This has been done because the styles do not directly map to the new presets. | ||
| * Please speak to your team's designer and update this to use a more appropriate preset. | ||
| */ | ||
| line-height: 1.15; |
dotcom-rendering/src/components/Masthead/Titlepiece/TitlepiecePillars.tsx
Outdated
Show resolved
Hide resolved
dotcom-rendering/src/components/Masthead/Titlepiece/titlepieceSharedStyles.ts
Outdated
Show resolved
Hide resolved
dotcom-rendering/src/components/Masthead/Titlepiece/TitlepieceSearchBar.tsx
Outdated
Show resolved
Hide resolved
| pillarUnderline, | ||
| isSelected && forceUnderline, | ||
| ]} | ||
| style={{ '--pillar-underline': pillarColour }} |
There was a problem hiding this comment.
We don't need to name all of these components with such long names now that they are within the ExpandedNav directory. We could call this file and component simply CollapseSectionButton for example
There was a problem hiding this comment.
With that in mind, are any of these different to the original implementation? I'm wondering if we actually need this duplication here. Could we just import the originals?
There was a problem hiding this comment.
Addressed with 803f79e. Re: could we just import the originals, I don't think so. The differences play out more clearly in the follow up (#11691). Having a completely separate set for the new-look masthead ought to allow for smoother implementation, then hopefully we can just remove the old bunch when the time comes
cemms1
approved these changes
Jul 1, 2024
There was a problem hiding this comment.
Great work in persevering through this! I've left a few comments but don't think anything is major. The main point here is that it would be great to make sure the PRs are as small as possible 😅 - this one was a little too long, even for someone with relevant context.
Happy to merge so long as we're sure there's nothing linked up to PROD code and it's all sitting behind a switch / in components not actually used by layouts yet.
frederickobrien
force-pushed
the
fairground/veggie-burger
branch
from
d17ab03
to
f88bc25
Compare
cemms1
force-pushed
the
fairground/titlepiece-use-nav-object
branch
from
095a8a4
to
b7f5a8c
Compare
An error occurred while trying to automatically change base from
fairground/titlepiece-use-nav-object
to
main
July 2, 2024 15:42
… for immediate future work
frederickobrien
force-pushed
the
fairground/veggie-burger
branch
from
803f79e
to
940f176
Compare
|
Seen on PROD (merged by @frederickobrien 9 minutes and 49 seconds ago) Please check your changes! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a new suite of
TitlePieceExpandedNavcomponents as part of the ongoing masthead work. After going down the wrong road for a while I've wound up replicating the oldNavcomponents to amend and build on, rather than trying to rebuild the whole thing from scratch.As part of making these new components (and Storybook stories) I've also created a /masthead directory for the various parts of Fairground's work to go. As a dev new to the repository I found it much easier to work in than a huge, flat approach.
Things not included in this PR but to be implemented in follow-ups:
Plus bugs we haven't had the pleasure of finding yet.
Screenshots
(There's not much to see here yet as the new components look much the same as the old ones. Updated designs are being implemented in #11691)