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

fix(ui): Fix entity-info styling #586

Closed
wants to merge 2 commits into from
Closed

fix(ui): Fix entity-info styling #586

wants to merge 2 commits into from

Conversation

machour
Copy link
Member

@machour machour commented Oct 26, 2017

Fixes unwanted UI changes introduced by #558

Before After
screen shot 2017-10-26 at 4 22 12 pm  screen shot 2017-10-26 at 4 21 18 pm

⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️

It seems that using styled-components the way it's done here does more harm than good. We didn't need to specify font-weight 'normal', or wrapperStyle in the past.
It's like using styled-component completely reset the style of the react-native-elements component, and we have to specify defaults again. Really weird.

Another painful thing is that before using SC, the hideChevron prop produced consistent margins no matter of its value. With SC, I got this difference for the first element:

screen shot 2017-10-26 at 4 11 18 pm

I had to display the chevron, but transparent for that element, to produce a consistent margin.

The big question here: Is SC used as it should be here? Is this a bug of SC ?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 34.457% when pulling 454a57d on machour:entity-info-fix into a9ae840 on gitpoint:master.

@andrewda
Copy link
Member

A bit random but @housseindjirdeh could you configure Coveralls to pass the Status Check as long as there's less than a 0.25% reduction in coverage? It's a bit silly to have it fail for this PR, or any PR that adds a tiny bit of complexity.

@andrewda
Copy link
Member

Actually not sure how I feel about that... Maybe we should keep it as is so we can be aware of when we're lowering coverage. Input?

Copy link
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.

Thanks for the quick fix on the problem, but yea it's interesting how this was fine w/o SC...

@machour
Copy link
Member Author

machour commented Oct 26, 2017

+1 on keep in it like that for now @andrewda

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 34.457% when pulling 109ae28 on machour:entity-info-fix into a9ae840 on gitpoint:master.

@andrewda
Copy link
Member

Alright, this should be fine to be merged. I'm adding a lot more tests for EntityInfo, but will follow those up in a future PR.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 35.839% when pulling 3918af6 on machour:entity-info-fix into 5266fb2 on gitpoint:master.

@machour
Copy link
Member Author

machour commented Oct 27, 2017

Finally had the time to investigate a bit more, there's an incompatibility between Styled-Components & React Native Elements where the default container style get lost.

More info in this bug report: https://github.com/react-native-training/react-native-elements/issues/679

@machour machour mentioned this pull request Oct 29, 2017
51 tasks
@machour
Copy link
Member Author

machour commented Nov 4, 2017

Closing this as #619 fixes the root cause

@machour machour closed this Nov 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants