-
Notifications
You must be signed in to change notification settings - Fork 6
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
DSD-1852: SubNav Menu Component #1697
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…design-system into DSD-1852/sub-nav-menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks for working on this and making all the updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question about the colors but otherwise looks and works great! Is there already a place where we're planning to use this?
import List from "../List/List"; | ||
import useScrollFadeStyles from "../../hooks/useScrollFadeStyles"; | ||
|
||
export const actionBackgroundColorsArray = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both arrays of colors are already declared in Banner
– could they live in one place now that two components are using them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@7emansell Yes, that is the eventual goal, but for now, we are continuing to use unique data objects in the separate components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor addition to the story for the hook, but otherwise, it looks good.
src/hooks/useScrollFadeStyles.mdx
Outdated
inline | ||
noStyling | ||
> | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
src/hooks/useScrollFadeStyles.mdx
Outdated
Link | ||
</SubNavLink> | ||
</li> | ||
</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
src/hooks/useScrollFadeStyles.mdx
Outdated
</li> | ||
</> | ||
</List> | ||
{showRightFade && <Box sx={styles.fadeEffect} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepikagonuguntla Can you also include a code example showing the styles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
Screen.Recording.2024-11-22.at.1.58.46.PM.movJust wondering - is the height of the component meant to grow as the screen shrinks? @deepikagonuguntla |
@oliviawongnyc Yes. There are two things happening when it hits the mobile viewport. First, the overall padding within the component jumps from |
I see, thanks, Marty! |
…design-system into DSD-1852/sub-nav-menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Fixes JIRA ticket (https://newyorkpubliclibrary.atlassian.net/browse/DSD-1852)
This PR does the following:
How has this been tested?
Accessibility concerns or updates
Accessibility Checklist
aria-live
is used, a screenreader was used to verify the text is read.ref
s, focus management was reviewed.Open Questions
Checklist:
Front End Review: