Add a prop to toggle ListView sticky section headers #11700

matt-oakes · 2017-01-02T14:50:09Z

Hello,

This PR adds a property to the ListView to enable and disable to sticky sections headers behaviour. Current this is enabled by default and there is no way to disable it. It has been previously discussed in #1974 where there was a suggestion to add the ListView inside ScrollView. This is bad for performance, but some people were using that as a workaround. @satya164 suggested someone submitting a PR, which is why I'm here 😉

I have tested the property manually by adding stickySectionHeaders={false} to the <ListView> Paging example in UIExplorer. I have also tested that the current behaviour still stands, so this is a non-breaking change.

I have also checked that the website displays the new documentation.

I couldn't see anywhere to add automated tests to this, but if there is feel free to point it out and I'll update this PR.

I tried running npm run lint to check the code style, but it spat out loads and loads of errors. I presume I have something set up incorrectly. Feel free to let me know if there's anything I need to fix.

Thanks,
Matt

facebook-github-bot · 2017-01-02T14:50:48Z

By analyzing the blame information on this pull request, we identified @jaredly and @nihgwu to be potential reviewers.

eslint-bot · 2017-01-02T14:58:04Z

Libraries/CustomComponents/ListView/ListView.js

@@ -472,9 +476,12 @@ var ListView = React.createClass({
    if (props.removeClippedSubviews === undefined) {


semi: Missing semicolon.

Fixed 👍 This comment was on the wrong line

satya164 · 2017-01-02T16:17:39Z

Libraries/CustomComponents/ListView/ListView.js

+     * is pushed off the screen by the next section header.
+     * @platform ios
+     */
+    stickySectionHeaders: React.PropTypes.bool,


This sounds like an array. Maybe stickySectionHeadersEnabled is a better name?

I can see what you mean. I was attempting to follow the convention set by removeClippedSubviews, but I guess it would be clearer as stickySectionHeadersEnabled. Should I make that change?

What's the purpose of this extra flag? I believe it's possible to check sectionHeaderIndices length instead. something like

if (sectionHeaderIndices.length) { stickyHeaderIndices.concat(sectionHeaderIndices); }

Does it make any sense?

@Kureev an extra flag is needed if we implement the sticky section header on Android as they have the different behavior by default
There is a discussion here #9957 and @janicduplessis did the same thing as @matto1990 did, though that PR was reverted then

I just checked the code (sorry for not doing it before writing my previous comment): it seems you can flag stickySectionHeadersEnabled (as @satya164 suggested) on line 419 and increment an index only if it's enabled. Then 479-484 won't be needed.

Thanks for the helpful insight, @nihgwu. It seems you knows more about this issue. I saw the last update on the followup PR has been made 28 days ago. Is there any work in progress?

@Kureev Previously the code would add in the indexes of all the sections headers whatever you set stickyHeaderIndices to. It did this by sending this.props.stickyHeaderIndices.concat(sectionHeaderIndices) to the ScrollView which the ListView uses. Because this happened within the ListView, there didn't seem to be a way to not have the headers sticky. The documentation also suggested this with the way it was worded. stickyHeaderIndices then essentially becomes the indexes of any additional views which you want to be sticky. Without this flag, section headers are always passed to the underlying ScrollView.

This flag basically makes that single line of logic run conditionally only if you want the headers to be sticky.

I can't see how that block of code would disable the sticky headers. Where are you proposing using it?

@Kureev there is another implementation #11315 but still in working

satya164 · 2017-01-02T16:18:41Z

@Kureev can you take a look too?

satya164 · 2017-01-02T16:43:07Z

Let's rename the prop and do what @Kureev suggested. Then we can merge.

nihgwu · 2017-01-02T16:46:51Z

I think stickySectionHeaders is OK 😄

Kureev · 2017-01-02T16:48:11Z

stickySectionHeaders doesn't sound like a flag. It sounds like array.

satya164 · 2017-01-02T17:20:16Z

@matto1990 I think what @Kureev is suggesting is at https://github.com/matto1990/react-native/blob/a10602a83c5ea420db098985f329e22ca9326f7d/Libraries/CustomComponents/ListView/ListView.js#L419, instead of

sectionHeaderIndices.push(totalIndex++);

we can do

if (this.props.stickySectionHeadersEnabled !== false) {
  sectionHeaderIndices.push(totalIndex++);
}

matt-oakes · 2017-01-02T17:23:34Z

I've just updated the name of the flag.

Sorry. I didn't see your reply @Kureev. the page had your first comment, but hadn't refreshed to show the subsequent ones 👍

I've made that change to conditionally add to the array on line 419, however, I have also renamed the array variable to stickySectionHeaderIndices from sectionHeaderIndices to make it clear that it's not the index of all of the headers.

satya164 · 2017-01-02T17:25:42Z

LGTM. @Kureev ?

DanielMSchmidt · 2017-01-03T09:21:21Z

Libraries/CustomComponents/ListView/ListView.js

@@ -229,6 +223,14 @@ var ListView = React.createClass({
     */
    removeClippedSubviews: React.PropTypes.bool,
    /**
+     * Makes the sections headers sticky.  The sticky behavior means that it


There are two spaces after the dot :)

I was following the style that I saw in the comment above (https://github.com/facebook/react-native/pull/11700/files#diff-b4c8e4c7b175f06ffa084d8dca28e86aL185), but I can see now that the double spacing is not consistent.

Should I change just my misuse or all double spaces in this face?

@matto1990 sure 👍

Looking through the files in Library/CustomCompnents there are lots of instances of .<space><space>. I don't think it's appropriate for this pull request to fix all of them. I will change the 10 instances in the ListView.js property comments, but I'll leave the copyright header with the double spaces so it is the same as the other headers in this folder.

It's not a big deal, but I don't want to muddy up this PR with unrelated style changes.

Kureev

LGTM

Kureev · 2017-01-04T13:16:24Z

@facebook-github-bot shipit

facebook-github-bot · 2017-01-04T13:17:31Z

@Kureev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

matt-oakes · 2017-01-09T09:17:11Z

What's the next step of the process with getting this merged in?

satya164 · 2017-01-09T09:54:11Z

@matto1990 seems this failed to import. I'll import it again. Can you rebase it against master? I merged master, but it'll then porbably show my name in commit after merged.

matt-oakes · 2017-01-09T10:08:03Z

@satya164 I have rebased and removed the merge commit.

satya164 · 2017-01-09T10:13:43Z

@facebook-github-bot shipit

facebook-github-bot · 2017-01-09T10:15:03Z

@satya164 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

matt-oakes · 2017-01-09T11:35:57Z

Perfect. Thanks @satya164!

matt-oakes · 2017-01-25T17:15:49Z

Can you confirm this will make it into 0.41? I can't see it on the changelog for the beta release.

satya164 · 2017-01-25T17:39:41Z

@matto1990 it'll probably be in 0.42

matt-oakes · 2017-01-25T17:47:54Z

@satya164 Thanks! Is there a way to see the status of this? I assumed that once this had been closed it was merged in.

satya164 · 2017-01-25T18:12:03Z

@matto1990 yes it's merged.

facebook-github-bot added the GH Review: review-needed label Jan 2, 2017

eslint-bot reviewed Jan 2, 2017

View reviewed changes

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 2, 2017

satya164 reviewed Jan 2, 2017

View reviewed changes

satya164 added GH Review: needs-revision and removed GH Review: review-needed labels Jan 2, 2017

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jan 2, 2017

DanielMSchmidt reviewed Jan 3, 2017

View reviewed changes

Kureev approved these changes Jan 4, 2017

View reviewed changes

Add a prop to toggle ListView sticky section headers

bf4add1

facebook-github-bot closed this in 5542756 Jan 9, 2017

kylekampy mentioned this pull request Jan 23, 2017

ScrollView rendered incorrectly with RefreshControl if push back a bit before finish refreshing #10747

Closed

matt-oakes deleted the listview__sticky_headers_prop branch February 9, 2017 15:19

		@@ -472,9 +476,12 @@ var ListView = React.createClass({
		if (props.removeClippedSubviews === undefined) {

Add a prop to toggle ListView sticky section headers #11700

Add a prop to toggle ListView sticky section headers #11700

Conversation

matt-oakes commented Jan 2, 2017 • edited Loading

facebook-github-bot commented Jan 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kureev Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satya164 commented Jan 2, 2017

satya164 commented Jan 2, 2017

nihgwu commented Jan 2, 2017

Kureev commented Jan 2, 2017 • edited Loading

satya164 commented Jan 2, 2017 • edited Loading

matt-oakes commented Jan 2, 2017

satya164 commented Jan 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kureev left a comment

Choose a reason for hiding this comment

Kureev commented Jan 4, 2017

facebook-github-bot commented Jan 4, 2017

matt-oakes commented Jan 9, 2017

satya164 commented Jan 9, 2017

matt-oakes commented Jan 9, 2017

satya164 commented Jan 9, 2017

facebook-github-bot commented Jan 9, 2017

matt-oakes commented Jan 9, 2017

matt-oakes commented Jan 25, 2017

satya164 commented Jan 25, 2017

matt-oakes commented Jan 25, 2017

satya164 commented Jan 25, 2017

matt-oakes commented Jan 2, 2017 •

edited

Loading

Kureev Jan 2, 2017 •

edited

Loading

Kureev commented Jan 2, 2017 •

edited

Loading

satya164 commented Jan 2, 2017 •

edited

Loading