-
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
Feature/use grid layout #25
Conversation
The “main” isn’t necessary
Navigation bar goes from left to top on small screens
<Route path="/" handler={MainScreen}> | ||
<Route name="settings" handler={SettingsRegion}/> | ||
<Route name="devices" handler={DevicesRegion}/> | ||
<Redirect from="/" to="devices"/> | ||
</Route> |
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 decided to get rid of subrouting for now?
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.
Yes, there's currently no need for it. Can be added if necessary
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.
Yep, anyway we do know how it works.
LGTM, there are however some comments |
Me as well |
var Device = require('screens/main/devices/device/index'); | ||
var Device = require('../device'); | ||
|
||
require('./style.scss'); |
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.
Why StyleMixin isn't used 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.
was not part of the PR, we worked on the main screen and navigation bar. This could be done by a separate PR
@dev-tim You should be unblocked after the merge