[Fairground 🎡] Expanded nav alignment and styling #11691
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: +114 B (+0.01%) Total Size: 867 kB
ℹ️ View Unchanged
|
3 tasks
frederickobrien
force-pushed
the
fairground/expanded-nav-alignment
branch
3 times, most recently
from
8a951b2
to
44b2116
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. |
mxdvl
reviewed
Jun 26, 2024
Comment on lines
15
to
32
| // 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; | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Are these comment left over for a specific reason?
|
|
||
| columnInput.addEventListener('keydown', function(e){ | ||
| // keyCode: 13 => Enter key | keyCode: 32 => Space key | ||
| if (e.keyCode === 13 || e.keyCode === 32) { |
There was a problem hiding this comment.
FYI keyCode is deprecated in favour of code
mxdvl
reviewed
Jun 26, 2024
dotcom-rendering/src/components/Masthead/Titlepiece/ExpandedNav/ExpandedNavColumn.tsx
Outdated
Show resolved
Hide resolved
dotcom-rendering/src/components/Masthead/Titlepiece/Titlepiece.tsx
Outdated
Show resolved
Hide resolved
Hey @mxdvl, sorry this one is for me to review and continue with. I think @frederickobrien might have forgotten to add me as a reviewer - have fixed that. Will take dotcom-platform off the list as this is hard to review without sufficient relevant context. I'll try to check in with the WebX team about these changes next week Useful info: this is a branch from another branch and when eventually merged, will be sitting behind a 0% test until ready. |
cemms1
force-pushed
the
fairground/veggie-burger
branch
from
ac8da14
to
6812e2d
Compare
cemms1
force-pushed
the
fairground/expanded-nav-alignment
branch
from
44b2116
to
b236842
Compare
cemms1
approved these changes
Jul 1, 2024
...endering/src/components/Masthead/Titlepiece/ExpandedNav/ExpandedNavCollapseSectionButton.tsx
Outdated
Show resolved
Hide resolved
| import { nestedOphanComponents } from '../../../../lib/ophan-helpers'; | ||
| import type { LinkType } from '../../../../model/extract-nav'; | ||
| import { palette as themePalette } from '../../../../palette'; | ||
| import { pillarWidthsPx } from '../titlepieceSharedStyles'; | ||
|
|
||
| const pillarHeight = 42; |
There was a problem hiding this comment.
maybe we could bring this into the config / constants file we have set up?
dotcom-rendering/src/components/Masthead/Titlepiece/ExpandedNav/ExpandedNavSections.tsx
Outdated
Show resolved
Hide resolved
dotcom-rendering/src/components/Masthead/Titlepiece/ExpandedNav/ExpandedNavSearchBar.tsx
Outdated
Show resolved
Hide resolved
dotcom-rendering/src/components/Masthead/Titlepiece/ExpandedNav/ExpandedNavSearchBar.tsx
Outdated
Show resolved
Hide resolved
frederickobrien
force-pushed
the
fairground/expanded-nav-alignment
branch
from
b236842
to
69df4e5
Compare
frederickobrien
force-pushed
the
fairground/veggie-burger
branch
from
803f79e
to
940f176
Compare
frederickobrien
force-pushed
the
fairground/expanded-nav-alignment
branch
from
c676cf4
to
a87945f
Compare
frederickobrien
force-pushed
the
fairground/expanded-nav-alignment
branch
from
c969955
to
e58569d
Compare
frederickobrien
force-pushed
the
fairground/expanded-nav-alignment
branch
from
e58569d
to
b687cc5
Compare
|
Seen on PROD (merged by @frederickobrien 8 minutes and 17 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.
Following on from #11572, this adjusts the alignment and styling of the new masthead expanded nav components to align (eh? eh?) with new designs.
The components are not reactive with this implementation (i.e. the expanded nav is constantly in the expanded state) but it looks as it should in that state. With a delicious new veggie burger all the pieces will be combined.
Some things to note:
palette.tshub rather than being imported directly from SourceWhat this does not do:
To be followed up with another PR in the chain.
Screenshots
Desktop
LeftCol
Wide