Make list headers sticky #700
Conversation
src/component/list.js
Outdated
| data: PropTypes.array.isRequired, | ||
| renderHeader: PropTypes.func, | ||
| renderItem: PropTypes.func.isRequired, | ||
| stickyHeaderIndices: PropTypes.array, |
There was a problem hiding this comment.
stickyHeaderIndices was added as a prop because I assumed we might not want to have sticky headers on all lists. If that's not the case we can hardcode stickyHeaderIndices={[0]} instead of line 53.
There was a problem hiding this comment.
IMO we do always want sticky headers :)
There was a problem hiding this comment.
Cool, I'll make the appropriate changes.
There was a problem hiding this comment.
@valentinewallace updated - all lists now have sticky headers.
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Looks great visually! Just one change really, I think we do always want sticky headers.
src/component/list.js
Outdated
| data: PropTypes.array.isRequired, | ||
| renderHeader: PropTypes.func, | ||
| renderItem: PropTypes.func.isRequired, | ||
| stickyHeaderIndices: PropTypes.array, |
There was a problem hiding this comment.
IMO we do always want sticky headers :)
| "react-art": "~16.3.2", | ||
| "react-dom": "~16.3.2", | ||
| "react-native-web": "^0.6.0", | ||
| "react": "^16.5.2", |
There was a problem hiding this comment.
Don't think these changes should be part of this PR?
There was a problem hiding this comment.
Because stickyHeaderIndices for ListView were buggy I had to upgrade react-native-web and its dependencies react, react-art, react-dom. Should we make the upgrade in a separate PR and rebase this one once the upgrade PR is merged?
There was a problem hiding this comment.
Don't think these changes should be part of this PR?
This is tricky. I've been putting off upgrading react/react-native-web since there was quite a bit of breakage in some views. @ERKarl have you tested all screens in the storybook to make sure there are no regressions? I remember ListViews not scrolling without a manual window resize after items were added dynamically by actions.
There was a problem hiding this comment.
Good point - I'll make sure to test this when I make the other requested changes in this PR.
There was a problem hiding this comment.
@tanx I tested the ListView scrolling by dynamically adding items - didn't see any issues.
As for testing the screens for visual regressions - I did test that when putting up the PR. The only regression I found was with the seed view which is fixed in this PR.
Please double test 🙏 .
There was a problem hiding this comment.
I tested the new dependency version. Looks like it breaks the seed view in storybook.
There was a problem hiding this comment.
This the output I'm getting in erkarl:sticky-list-headers branch:
Can you elaborate on what's broken here?
There was a problem hiding this comment.
This is on my machine after installing the versions in the package.json
There was a problem hiding this comment.
Ok I upgrade react/react-native-web in a separate PR and merged to master. @ERKarl would you kindly rebase this PR on top of the master? thanks
|
@tanx rebased. Should we also squash the commits? |
tanx
left a comment
tanx
left a comment
There was a problem hiding this comment.
LGTM... just one minor nit.
| ref={component => (this.list = component)} | ||
| dataSource={this.dataSource} | ||
| renderHeader={this.props.renderHeader} | ||
| stickyHeaderIndices={[0]} |
There was a problem hiding this comment.
Since we're setting this globally for all List instances, we'll also need to add style={{ backgroundColor: color.white }} to the ListHeader in list-story in storybook.
ghost
deleted the
2 participants
In this PR we:
transactions,channelsandnotificationslist headers stickyreact-native-weband its dependencies (react-art,react-dom,react-dom)The upgrade for
react-native-webwas required because previouslystickyHeaderIndicesforListViewwas buggy. It's worth noting that the new version also supports FlatList and SectionList which have lots of improvements over theListView.During the upgrade process, I verified that all the screens and components didn't visually change/break. The only change needed was for the
Seedview becauseflexGrowproperty used inMainContentcomponent now works according to the spec - it's assigningflexBasistoauto. I've included the necessary fix forSeedview.In the future we could add visual regression tests to the storybook stories to make sure the views don't change when upgrading libraries etc.
Channels:
Transactions:
Notifications:
Closes: #611