-
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
Add a basic FTU #42
Add a basic FTU #42
Conversation
4a2380b
to
5debd42
Compare
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.
Looking pretty nice.
Should we use this react-native-viewpager component as our Carousel? #12
import Home from './scenes/Home'; | ||
import Item from './scenes/Item'; | ||
import Map from './scenes/Map'; | ||
import Profile from './scenes/Profile'; |
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.
Any reason for this reordering?
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 really missing that we cannot do a dynamic import :( :( :( would love to load all this scenes like fte, debug, etc. on demand.
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.
The new order reflects the order the routes are declared in the constructor below.
Also I know it's bad to import everything but I wouldn't worry too much as the components are not instantiated.
In the final product, I think it makes sense to be able to access the FTU (remember Firefox OS?)
@@ -0,0 +1,80 @@ | |||
import React, { PropTypes } from 'react'; |
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 going to be a bit picky, could you add some documentation explaining what this component does?
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.
It has now better documentation.
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.
Thanks, much better :)
js/components/ViewPagerIndicator.js
Outdated
const DOT_SIZE = 12; | ||
const DOT_SPACE = 6; | ||
|
||
const ViewPagerIndicator = ({ |
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.
nit:
export default const ViewPageIndicator ...
|
||
this.navigator = this.props.navigator; | ||
|
||
const PAGES = [ |
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.
Can we render the pages on demand? Or are we going to force to show always the two pages?
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.
The pages are rendered on demand. The code used to build each page is only called when required (think of <ListView>
component).
The API seems flexible so it can also handle complicated case where you go to different pages depending on which button you tap.
js/scenes/FTU.js
Outdated
this.renderPage2(), | ||
]; | ||
const dataSource = new ViewPager.DataSource({ | ||
pageHasChanged: (p1, p2) => p1 !== p2, |
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 hate that RN doesn't have this datasource definition by default, I think we have the same filter in the home with the list view.
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.
Nice work @gmarty! Comments:
-
I'm unable to swipe the view-pager. It's very confusing as the dots suggest to me that the view is swipeable. If it's not, I'd suggest removing the dots.
-
We've introduced a new button design here. I suggest we have a single primary-call-to-action button that is used throughout the app. This button needs to be able to accept text and icons.
-
Action hierarchy UX: IMO we should have one clear 'primary' call to action and downgraded 'secondary' call-to-action. For example 'Turn On' is primary and 'Skip' is secondary.
-
Page indicators have no vertical padding:
js/components/ViewPagerIndicator.js
Outdated
} from 'react-native'; | ||
|
||
const { width } = Dimensions.get('window'); | ||
const DOT_SIZE = 12; |
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.
Dots are quite big, can you refer to Android home screen for normal dot sizes.
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 referred to the UI specs by Francis.
What about 10 px?
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.
These are only UX specs, not visual, so don't worry about implementing them to pixel perfection. Android page indicators look half the size to me.
* @param scrollOffset {number} Used to reset dot position after a transition. | ||
* @return {Component} | ||
*/ | ||
const ViewPagerIndicator = ({ |
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 understand this a shorthand declaration, but I'd rather we just be consistent with how we define components (ie. class
).
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 more than just a shorthand. These stateless functional components are the recommended way to build components that are purely visual. They've been added in React 0.0.14 and they're likely to get an optimisation boost in the next versions. Ideally I'd like all the components in the components
folder to use this syntax.
You can read more in the announcement blog and this blog post.
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.
Interesting thanks!
js/components/ViewPagerIndicator.js
Outdated
// Initial left position of the active dot used during transition. | ||
const offsetX = (width - DOT_FULL_WIDTH * pageCount) / 2 + | ||
(activePage - scrollOffset) * DOT_FULL_WIDTH; | ||
// The CSS `left` property. |
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.
NIT: Line break before comments. Is there a rule for this?
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's no rule as of yet, but feel free to make a PR or open an issue.
source={require('../images/ftu/background.jpg')} | ||
resizeMode="cover" | ||
style={styles.container}> | ||
<View style={styles.scrim}> |
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 a scrim
?
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.
n. A transparent fabric used as a drop in the theatre to create special effects of lights or atmosphere.
That's a word that came up when I was working on the settings drawer of Firefox OS. I think @djabberwocky was working on the spec back then so he may know more about this term.
That may not be appropriate in this case.
js/scenes/FTU.js
Outdated
return ( | ||
<View style={styles.screen}> | ||
<Text style={styles.title}>{`Hear the story of London's vibrant street art`.toUpperCase()}</Text> | ||
<Text style={styles.text}>This month Project Magnet brings you a selection of amazing street art from around London.</Text> |
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.
Should we move this text out into a config file?
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 agree this is not pretty. Maybe if I move it to a constant at the beginning of the file it will make it easier to read.
title="GET STARTED" | ||
accessibilityLabel="Continue to the next screen." | ||
onPress={() => this.refs.viewPager.goToPage(1)} | ||
color={Platform.OS === 'ios' ? 'white' : 'transparent'}/> |
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 our current design we have a black button with white border as our primary-call-to-action button. Can we be consistent and have a single <PrimaryButton>
component that we use everywhere?
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 after our UX discussions of yesterday, let's not focus too much on design. All will change.
I agree we need a specific button. Let's open an issue for this.
|
The dots are cut out for me too. I will land this patch, it will change a lot once slicebread comes with the design, but we can start wiring the logic for launching it just one time and so on. Also it can be used in the user study as a place holder until we have the final visuals. But yes the dots ... could be completely visible :) |
Oh yes I forgot to comment on the dots. They are currently cut out because this patch works with #49 so that the FTU is displayed full screen with no background on the status bar (take a look at the slightly outdated visual posted above). |
Landed on master |
This PR contains:
What is needed for the FTU now but will be on another PR: