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

React Native <0.43 support #1555

Merged
merged 4 commits into from Jul 31, 2017
Merged

React Native <0.43 support #1555

merged 4 commits into from Jul 31, 2017

Conversation

atticoos
Copy link
Member

@atticoos atticoos commented Jul 31, 2017

Issue: Builds were broken for React Native versions <0.43. (#1553)

What I did

Swapped out SectionList for ListView. This will be deprecated in the future (I don't believe anytime soon/immediate), so ideally we may want to export a List component that interfaces with whichever underlying ListView / SectionList implementation is supported. For now, this simply unbreaks <0.43 builds.

There are two additional regressions: StyleSheet.absoluteFillObject and the use of maxWidth in a style. For ease of use moving forward, I added a polyfills/ directory to address these head on.

StyleSheet.absoluteFillObject

This is only supported in recent versions. It's simply a preset style for a full screen absolutely positioned element.

MinMaxViewPolyfill

The min/max width/height features of styles are not available on earlier versions of React Native. This adds a Higher Order Component that can declaratively (through props) apply these bounding rules to any child component.

How to test

Run the react-native-vanilla project and verify all continues to work.

Proof

screen shot 2017-07-31 at 1 35 46 pm

Closes #1553

@atticoos atticoos force-pushed the bugfix/react-native-section-list branch from a03f19d to 8923341 Compare July 31, 2017 17:38
@codecov
Copy link

codecov bot commented Jul 31, 2017

Codecov Report

Merging #1555 into master will decrease coverage by 0.32%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1555      +/-   ##
==========================================
- Coverage   20.46%   20.14%   -0.33%     
==========================================
  Files         241      244       +3     
  Lines        5243     5327      +84     
  Branches      649      660      +11     
==========================================
  Hits         1073     1073              
- Misses       3678     3744      +66     
- Partials      492      510      +18
Impacted Files Coverage Δ
...tive/src/preview/components/StoryListView/style.js 0% <ø> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/style.js 0% <0%> (ø) ⬆️
...tive/src/preview/components/StoryListView/index.js 0% <0%> (ø) ⬆️
...ive/src/preview/components/polyfills/MinMaxView.js 0% <0%> (ø)
...t-native/src/preview/components/polyfills/index.js 0% <0%> (ø)
...ive/src/preview/components/polyfills/StyleSheet.js 0% <0%> (ø)
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
...ponents/left_panel/stories_tree/tree_decorators.js 30.88% <0%> (ø) ⬆️
... and 21 more

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 e3bcd4d...73ac57e. Read the comment docs.

@rmevans9
Copy link
Contributor

This looks good to me. I personally dislike having to "go backwards" but I understand the need.

rmevans9
rmevans9 previously approved these changes Jul 31, 2017
@atticoos
Copy link
Member Author

I just realized I hand't audited other components. Verifying StatusBar is supported in 0.27

@atticoos
Copy link
Member Author

StatusBar is supported in 0.27. I think we're all good here.

I should pull this into my 0.27 project and verify that it all works as expected prior to merging.

shilman
shilman previously approved these changes Jul 31, 2017
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM! Tested (but not on old RN)

@atticoos
Copy link
Member Author

Testing RN 0.27 right now. Will report back shortly

@atticoos
Copy link
Member Author

Have some minWidth and maxWidth problems:

screen shot 2017-07-31 at 1 58 22 pm

WIP

- StyleSheet.absoluteFillObject polyfill
- Polyfill for min/max width/height style rules through a component
@atticoos
Copy link
Member Author

Ran into two additional backward incompatibilities. For ease of use moving forward, I added a polyfills/ directory to address these head on.

StyleSheet.absoluteFillObject

This is only supported in recent versions. It's simply a preset style for a full screen absolutely positioned element.

MinMaxViewPolyfill

The min/max width/height features of styles are not available on earlier versions of React Native. This adds a Higher Order Component that can declaratively (through props) apply these bounding rules to any child component.

@atticoos atticoos dismissed stale reviews from rmevans9 and shilman July 31, 2017 18:52

Made additional changes

@atticoos
Copy link
Member Author

atticoos commented Jul 31, 2017

38ef3f6 moves the polyfill implementations into react-native-compat

@atticoos atticoos force-pushed the bugfix/react-native-section-list branch from d5af172 to 38ef3f6 Compare July 31, 2017 22:24
@rmevans9
Copy link
Contributor

👍

How far back do we want to support? I understand we want to make sure that we support older versions of React Native but there has to be a limit to how far back we go.

@atticoos
Copy link
Member Author

atticoos commented Jul 31, 2017

How far back do we want to support? I understand we want to make sure that we support older versions of React Native but there has to be a limit to how far back we go.

It's all about finding the proper balance. Storybook itself doesn't require much UI, so with minimal effort we're able to keep folks on earlier versions supported. If there's any drawback, I think it'll be on the contributor and reviewer side to keep those requirements communicated and enforced.

We currently support >=0.27, so for this release at the very least, we want to make sure we continue to satisfy that requirement without regressions.

I don't think this is the proper thread to make a formal decision, but it is something to consider moving forward with the compatibility support in, and observe how things go in future reviews.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM and tested

@shilman shilman merged commit 85de359 into master Jul 31, 2017
@shilman shilman deleted the bugfix/react-native-section-list branch July 31, 2017 23:29
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.

None yet

3 participants