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 Header (+ Refactor) #1221

Merged
merged 13 commits into from
Sep 23, 2018
Merged

Fix Header (+ Refactor) #1221

merged 13 commits into from
Sep 23, 2018

Conversation

martinezguillaume
Copy link
Collaborator

@martinezguillaume martinezguillaume commented May 23, 2018

@react-native-elements-ci
Copy link
Collaborator

react-native-elements-ci commented May 23, 2018

Copy link
Collaborator

@iRoachie iRoachie left a comment

Choose a reason for hiding this comment

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

Snapshots bro!

@codecov
Copy link

codecov bot commented May 23, 2018

Codecov Report

Merging #1221 into next will decrease coverage by 1.09%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##             next    #1221     +/-   ##
=========================================
- Coverage   72.26%   71.17%   -1.1%     
=========================================
  Files          33       30      -3     
  Lines         595      569     -26     
  Branches       90       84      -6     
=========================================
- Hits          430      405     -25     
+ Misses        138      137      -1     
  Partials       27       27
Impacted Files Coverage Δ
src/header/Header.js 87.5% <80%> (-4.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a574c2...6c4653e. Read the comment docs.

Copy link
Collaborator

@iRoachie iRoachie left a comment

Choose a reason for hiding this comment

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

The left component is taking up way too much space.

@iRoachie
Copy link
Collaborator

Compared to the picture we have in the docs

@@ -34,13 +35,15 @@ describe('Header Component', () => {
</Header>
);

expect(component.find('Button').length).toBe(2);
expect(component.length).toBe(1);
expect(toJson(component)).toMatchSnapshot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be snapshotting here ?. Can we test the actually functionality describe by the test case 😉
should render multiple children when passed in

expect(component.children().length).toBe(2)

or more explicitly

expect(component.find('Button').length).toBe(2)

same with the other cases :)

@martinezguillaume
Copy link
Collaborator Author

@iRoachie It's fixed
@Gregjarvez I revert the test changes

@iRoachie
Copy link
Collaborator

@martinezguillaume Mind doing the typescript definitions or you want me to do em?

@martinezguillaume
Copy link
Collaborator Author

@iRoachie I’ll do it, I forgot 🙂

@xavier-villelegier
Copy link
Collaborator

Ping @martinezguillaume 🔔


```js
<Header
statusBarProps={{ barStyle: 'light-content' }}
barStyle="light-content" // or directly
Copy link
Collaborator

Choose a reason for hiding this comment

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

@martinezguillaume Any reason you added this?

Copy link
Collaborator Author

@martinezguillaume martinezguillaume Jul 31, 2018

Choose a reason for hiding this comment

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

Because I think it’s a very used props of StatusBar component. So we can use it directly without doing

<Header
  statusBarProps={{ barStyle: 'light-content' }}
/>

@iRoachie iRoachie merged commit d574d3b into next Sep 23, 2018
@iRoachie iRoachie deleted the ft/header branch September 23, 2018 18:58
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.

5 participants