-
Notifications
You must be signed in to change notification settings - Fork 24
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
Demo App - Adds Navigational sidebar to layouts #4730
Conversation
|
✅ Deploy Preview for unrivaled-bublanina-3a9bae ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
}) => { | ||
const theme = useContext(ThemeContext); | ||
const breakpoint = useContext(ResponsiveContext); | ||
return ( | ||
<Page kind="full"> | ||
<Page kind="full" {...rest}> |
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.
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.
with updated theme styling this elevation goes away anyway, but for toggling between themes
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.
done.
label={ | ||
<Box direction="row" align="center" justify="between"> | ||
<Box direction="row" align="center" gap="xsmall"> | ||
<Box width={global.edgeSize.medium}> |
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.
I'm comfortable with this. Would it make sense to have the width of this be set to medium.hpe.size.icon.medium
? Meaning we're reserving space directly for the icon's dimension.
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.
Hmmm... Yes, that seems appropriate. I was originally thinking we'd replace it with a content size token value, but icon size seems more precise.
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.
done.
<Page pad={{ bottom: "xlarge" }}> | ||
<PageContent alignSelf="start"> | ||
<PageHeader | ||
title="Navigational Sidebar" |
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.
title="Navigational Sidebar" | |
title="Navigational sidebar" |
sentence casing since it's not a product name.
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.
done.
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.
Overall looks quite nice. A few minor comments.
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, thanks!
Deploy Preview
What does this PR do?
Demo App
Where should the reviewer start?
What testing has been done on this PR?
In addition to the feature you are implementing, have you checked the following:
General UX Checks
Accessibility Checks
Code Quality Checks
How should this be manually tested?
Any background context you want to provide?
What are the relevant issues?
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
Is this change backwards compatible or is it a breaking change?