Skip to content

refactor: migrate notifications screen to styled-components#684

Merged
housseindjirdeh merged 1 commit intogitpoint:masterfrom
ZahraTee:styled-components-notifications-screen
Jan 3, 2018
Merged

refactor: migrate notifications screen to styled-components#684
housseindjirdeh merged 1 commit intogitpoint:masterfrom
ZahraTee:styled-components-notifications-screen

Conversation

@ZahraTee
Copy link
Copy Markdown
Contributor

Screenshots

Before After
before-none after-none
before-notifications after-notifications

Part of #532.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 40.544% when pulling 2dad11e on ZahraTee:styled-components-notifications-screen into 5691f00 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.

A little question here too :)

]}
>
<Text style={styles.noneTitle}>
<TextContainer height={contentBlockHeight}>
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.

Shouldn't TextContainer styled definition read and apply the height prop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought that too but I found that you actually don't need to... It seems that styled.Views can apply values (including height) passed in as props directly and handles null values better. I built out a small example here: https://snack.expo.io/Bys9FZumz

I can change it if it's more explicit but I thought leaving it like this is a bit neater.

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.

styled-components pass on all their props.

From documents, I learned something new. :)

Copy link
Copy Markdown
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

This looks perfect, thank you @ZahraTee 🎉

@housseindjirdeh housseindjirdeh merged commit 4c5ba60 into gitpoint:master Jan 3, 2018
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