Skip to content
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

Full screen experience - Content scroll underneath transparent status bar #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gmarty
Copy link
Contributor

@gmarty gmarty commented Jan 31, 2017

Full screen experience for added beauty.

See #43.

@@ -136,9 +135,10 @@ export class App extends Component {
renderScene({ id, data }, navigator) {
switch (id) {
case 'item': return <Item itemId={data.itemId} navigator={navigator}/>;
default:
var Component = this.routes[id].component;
default: {
Copy link
Member

@arcturus arcturus Jan 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the curly brackets needed? I'm just curious.

Copy link
Contributor Author

@gmarty gmarty Feb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's to create a new scope to allow the use of const.

@@ -81,11 +83,13 @@ Header.propTypes = {
navigator: PropTypes.object,
};

Header.HEIGHT = 54;
const STATUS_BAR_HEIGHT = Platform.OS === 'ios' ? 20 : StatusBar.currentHeight;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the values 20 and 54 constants with self explained name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

20 is the height of the status bar on iOS. In Android, we can just use StatusBar.currentHeight.
54 is the height we want the header to have. This should be pretty self explanatory already.

@arcturus
Copy link
Member

lgtm, left some nit comments.

@wilsonpage
Copy link
Collaborator

I'm not sure this is ok:

screen shot 2017-01-31 at 09 48 01

Perhaps we need to pad the list-item content in a bit more?

@wilsonpage
Copy link
Collaborator

Also on Android:

screen shot 2017-01-31 at 09 50 21

Overlapping components is not OK IMO. We may be able to pad in the list-item contents a little more and add some kind of semi-opaque black background to the system status-bar. But I fear that this might look strange still.

Copy link
Collaborator

@wilsonpage wilsonpage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't looking right at the moment. An Android status bar with lots of notification icons is going to look messy overlaid on top of the publisher profile and star components.

Wondering if there is a way to scroll the content under the status bar but have it offset by the height of the status-bar when scrollTop is 0. The hidden header UI will make this complex to implement.

@gmarty
Copy link
Contributor Author

gmarty commented Jan 31, 2017

I reckon it looks weird, but I thought that'd help for the discoverability of the hidden header.
I'll keep working on this PR and see what I can do to make it look better.

@wilsonpage
Copy link
Collaborator

I was drinking a coffee and had an idea! All we have to do is increase the paddingTop for the first list item. That way it will look neat when at scroll-top, works with the hidden heading interaction and is hidden out of view when list is at any other scroll position.

@gmarty
Copy link
Contributor Author

gmarty commented Feb 1, 2017

So in this update, I added a grey margin on top of the first item, but then scrolling down will reveal the header. I had to reduce the header to subtract the height of the status bar and fix the vertical positioning. It's not as hackish as it sounds and the result is nice.
The default view still show a dark header on top of the first item so it's not as nice as before but at least here all items have exactly the same template.

Let's land this now and we can always revisit it later.

@gmarty gmarty mentioned this pull request Feb 1, 2017
Copy link
Collaborator

@wilsonpage wilsonpage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're almost there, but can't land this if the firstRow element is blocking touches on <Header>

@@ -121,7 +120,7 @@ export class App extends Component {
<View style={styles.container}>
<StatusBar
translucent={true}
backgroundColor="rgba(0, 0, 0, 0.2)"
backgroundColor="transparent"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep a bit of background color so that if status-bar icons are over a white background (in our app) they are still visible?

if (y > Header.HEIGHT) return {
value: Header.HEIGHT,
offset: y - Header.HEIGHT,
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really funky syntax, can you find an alternative :-/

@@ -436,6 +442,12 @@ export class Home extends Component {
);
}

renderHeader() {
return (
<View style={styles.firstRow}/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This element overhangs the header content and blocks the header buttons. 'Settings' button is difficult to press.

inputRange: [0, Header.HEIGHT],
outputRange: [0, Header.HEIGHT],
inputRange: [0, Header.HEIGHT - Header.STATUS_BAR_HEIGHT],
outputRange: [0, Header.HEIGHT - Header.STATUS_BAR_HEIGHT],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this calculation once?

const MAX_Y = Header.HEIGHT - Header.STATUS_BAR_HEIGHT;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants