[BOOKINGSG-9244][RYN] migrate navbar component#1171
Open
ryan-nguyen-t wants to merge 22 commits into
Open
Conversation
ryan-nguyen-t
force-pushed
the
BOOKINGSG-9244/navbar
branch
from
4ca7093 to
989818a
Compare
ryan-nguyen-t
requested review from
ghazwan-gt,
iant-gds,
zhaoyanxzy and
ziggyzet
ghazwan-gt
reviewed
May 13, 2026
The secondary brand is overflowing |
Author
|
@ghazwan-gt What screen size are you referring to for the overflowing brand |
ryan-nguyen-t
force-pushed
the
BOOKINGSG-9244/navbar
branch
from
ad2f991 to
9910eb5
Compare
ziggyzet
approved these changes
May 13, 2026
added 8 commits
May 14, 2026 09:51
qroll
reviewed
May 14, 2026
ryan-nguyen-t
force-pushed
the
BOOKINGSG-9244/navbar
branch
from
9e01ab6 to
fec29a6
Compare
qroll
requested changes
May 14, 2026
Comment on lines
74
to
76
| ${MediaQuery.MaxWidth.lg} { | ||
| margin-left: 0rem; | ||
| } |
Contributor
There was a problem hiding this comment.
treat as 0 by default, this media query can be removed from this class
qroll
reviewed
May 14, 2026
Comment on lines
+47
to
+61
| export const navCompressed = css` | ||
| height: ${ComponentToken.Navbar["compressed-height"]}; | ||
| `; | ||
|
|
||
| export const navFull = css` | ||
| height: ${ComponentToken.Navbar["full-height"]}; | ||
| `; | ||
|
|
||
| export const navDarkCompressed = css` | ||
| height: calc(${ComponentToken.Navbar["compressed-height"]} - 1px); | ||
| `; | ||
|
|
||
| export const navDarkFull = css` | ||
| height: calc(${ComponentToken.Navbar["full-height"]} - 1px); | ||
| `; |
Contributor
There was a problem hiding this comment.
as discussed in CR, we can apply the height as a variable to reduce the number of combinations
Contributor
There was a problem hiding this comment.
there's no need to use useApplyStyle for this, you should be able to apply the variable via css as well
export const navCompressed = css`
${token.nav.height}: ${ComponentToken.Navbar["compressed-height"]};
`;
export const navDark = css`
// ...
`
qroll
reviewed
May 14, 2026
ryan-nguyen-t
force-pushed
the
BOOKINGSG-9244/navbar
branch
from
fc8bfb5 to
4847434
Compare
added 6 commits
May 14, 2026 15:27
ghazwan-gt
approved these changes
May 15, 2026
qroll
reviewed
May 15, 2026
Comment on lines
+349
to
+351
| !hideNavBranding | ||
| ? styles.navElementsContainerWithBranding | ||
| : null |
Contributor
There was a problem hiding this comment.
!hideNavBranding && styles.navElementsContainerWithBranding
Contributor
There was a problem hiding this comment.
hmmm why is the masthead missing in some cases?
Author
There was a problem hiding this comment.
I think it was the issue with running parallel tests. Rerunning the test can gen the masthead. Sometimes the snapshots are not consistent...
Author
There was a problem hiding this comment.
updated: rerunning can only capture masthead for some but not all missing test cases, so I added a function to wait for the masthead to render first
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Checklist
classNameis chained correctly withclsx