Skip to content

refactor: parallax scroll to styled components#607

Merged
machour merged 6 commits intogitpoint:masterfrom
simonhoyos:refactor-parallax-scroll
Jan 26, 2018
Merged

refactor: parallax scroll to styled components#607
machour merged 6 commits intogitpoint:masterfrom
simonhoyos:refactor-parallax-scroll

Conversation

@simonhoyos
Copy link
Copy Markdown
Contributor

Screenshots

Before After
screen shot 2017-10-29 at 14 08 02 screen shot 2017-10-29 at 14 19 45
screen shot 2017-10-29 at 14 08 21 screen shot 2017-10-29 at 14 20 02
screen shot 2017-10-29 at 14 08 27 screen shot 2017-10-29 at 14 20 14

Description

Migrated ParallaxScroll to styled-components

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 39.489% when pulling af84d86 on shmesa22:refactor-parallax-scroll into 2fda53e on gitpoint:master.

Copy link
Copy Markdown
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

@shmesa22 looks good aside from the small comment I left you. Nice work.

const Background = styled.View`
position: absolute;
top: 0;
background-color: red;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Debug leftover? :)

Copy link
Copy Markdown
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

Other than what @machour pointed out, looks good!

const Background = styled.View`
position: absolute;
top: 0;
background-color: red;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be colors.primaryDark I believe.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 39.489% when pulling f591fe4 on shmesa22:refactor-parallax-scroll into 2fda53e on gitpoint:master.

Copy link
Copy Markdown
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

@shmesa22 had more time to review, left you two more changes request.

Could you take care of those? I'll merge this baby in once done.

Thank you!

position: absolute;
top: 0;
background-color: ${colors.primaryDark};
${props => props.style};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@shmesa22 after a second read, as style now only contain the height, could you rename the prop to height, and use it in this style? (height: ${props => props.height})

I think it would be more clear


const StickySectionText = styled.Text`
color: ${colors.white};
${fonts.fontPrimaryBold};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use styledFonts instead of fonts from config/, with font-family: ${styledFonts.fontPrimaryBold}

@machour machour self-assigned this Nov 4, 2017
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 39.987% when pulling 091737b on shmesa22:refactor-parallax-scroll into 7fc0681 on gitpoint:master.

Copy link
Copy Markdown
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

Hi @shmesa22

Sorry for the long time I took getting back to you 😞
I tested your PR and is working fine as long as you substitute styledFonts with fonts (we removed styledFonts this week)

Left you review comments to explain the changes, could you take care of them? 🙏

I'd happily merge this PR and do the changes myself if you're too busy, no worries!

import styled from 'styled-components/native';

import { colors, fonts, normalize } from 'config';
import { colors, normalize, styledFonts } from 'config';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use fonts instead of styledFonts (removed)


const StickySectionText = styled.Text`
color: ${colors.white};
font-family: ${styledFonts.fontPrimaryBold};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of:
font-family: ${styledFonts.fontPrimaryBold};

use:
${fonts.fontPrimaryBold}; (without specifying font-family)

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 44.118% when pulling c71fb7d on shmesa22:refactor-parallax-scroll into 0d694bf on gitpoint:master.

@machour machour merged commit 2a2e5a3 into gitpoint:master Jan 26, 2018
@machour
Copy link
Copy Markdown
Member

machour commented Jan 26, 2018

Thank you @shmesa22 !!

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.

6 participants