-
Notifications
You must be signed in to change notification settings - Fork 3
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
Desktop Header Component #9
Conversation
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.
Nicely done @savannahostrowski ! 🙌
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 great! Lots of questions as I continue to learn the art of making things look good, and one mega-nit:
.logo { | ||
height: 120px; | ||
width: 120px; | ||
background: url('assets/logo.png') no-repeat; |
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.
💯
src/components/DesktopHeader.js
Outdated
<Button className={classes.moreIcon} onClick={this.toggleDrawer('right', true)}><Menu/></Button> | ||
<SwipeableDrawer | ||
disableBackdropTransition={!iOS} | ||
disableDiscovery={iOS} |
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.
What's going on here?
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.
Refer below!
src/components/DesktopHeader.js
Outdated
|
||
}; | ||
|
||
const iOS = process.browser && /iPad|iPhone|iPod/.test(navigator.userAgent); |
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.
Do iPhones or iPods ever get the Desktop view anyways?
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.
So this was a problem with Material UI and how iOS devices handle the Swipeable Drawer component. I copied this logic from OSM Quality Web but tracked down a GitHub issue that outlines some of the issues that were happening.
I'm going to remove this logic for now but when we test on iOS, let's check this out.
</div> | ||
</SwipeableDrawer> | ||
<div className="nav"> | ||
<div id="explore" className="categories" onClick={() => history.push('/map')}><h4>Explore</h4></div> |
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.
🔥
src/App.css
Outdated
} | ||
} | ||
|
||
@media (min-width: 1440px) { |
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 is really slick. Are 960/1440px pretty standard values for these kinds of breakpoints? Also 20/24/27% or did you come up with that on your own?
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.
Mmm, I guess we should actually standardize these some more.
Typically we use 768px and 1280px etc. (https://medium.com/@uiuxlab/the-most-used-responsive-breakpoints-in-2017-of-mine-9588e9bd3a8a)
I'm going to rework these a bit.
The percentages were defined by me to accommodate the breakpoints.
} | ||
} | ||
|
||
.logo { |
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.
One mega-mega-mega nit is that I don't think this logo has a drop-shadow like the rest of the header does. IDK how tricky that is to make happen -- maybe something for the backlog?
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 added a v. small box shadow for now on 3 sides...not perfect because it overlaps the black bar but I'm not 100% sure that we can do this partially with CSS. Alternatively, I can remove the shadow from the black bar.
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.
Also, we need a better asset for the logo - this is a screenshot 😆
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.
We also might want to throw an onClick event on the logo to nav back to home eventually but that's tbd.
e7da563
to
a8e8182
Compare
7d812f1
to
bd76cbd
Compare
This PR contains a header component for desktop. This PR should be merged AFTER #8 as I will need to clean some things up once that one is merged for consistency's sake.