-
Notifications
You must be signed in to change notification settings - Fork 1
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
modify language button display location #4
base: master
Are you sure you want to change the base?
Conversation
src/components/YepsNavBar.css
Outdated
border: 1px solid transparent; | ||
border-radius: 4px; | ||
} | ||
@media (min-width: 768px) { |
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.
Where is 768px from? Most mobile device has width of 480px or lower. Also hardcoding pixel is not responsive design.
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.
Get this value from chrome inspect
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.
Let's not hard code. I can think of two ways: 1) detect the device type 2) use the DOM object to get its current dev width.
src/components/YepsNavBar.css
Outdated
.test-language-button { | ||
position: relative; | ||
float: right!important; | ||
margin-top: 8px; |
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.
Let's not hard code pixels on margin either. Maybe at least use percentage. Ideally, I think there may be some out-of-the-box solution from react-bootstrap.
src/components/YepsNavBar.tsx
Outdated
@@ -31,10 +30,12 @@ export default class YepsNavBar extends React.Component<Props, State> { | |||
<img className={'logo-wrapper'} src={logo} alt="logo"/> | |||
</Navbar.Brand> | |||
<Navbar.Toggle onClick={this.expandCollapse}/> | |||
<Button className={'language-button, test-language-button'} type="submit">Language</Button> |
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.
You may be able to achieve ideal button positioning by using Narbar.Form with pullLeft, just like below. Also, latter on we will need to unilaterally deal with the form submit with some state transition, in maybe redux store. Let me know if there's any difficulty wrapping button around in the Navbar.Form
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.
Left some comments. Mainly thinking of responsive design, since the design principle in this website is mobile first - we expect around ~90% of traffic from mobile.
Also, think about how to handle button click as a form post.
Good job anyway!
… collapse-expand context instead of checking the device source, for better accuracy. If the expand is undefined, then the nav items must not be collapsed, a golden source of truth
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.
Let's check in first.
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.
Doesn't work. Let's not merge this one.
Add 2 button to display at different screen width condition