Skip to content

Conversation

fangpenlin
Copy link

@fangpenlin fangpenlin commented Oct 19, 2024

Summary:

Fix #18945 inverted sticky headers (footers actually) not working issue

Changelog:

[General] [Fixed] - Fix SectionList sticky footer doesn't work as expected when the list has inverted flag set

Test Plan:

Screen.Recording.2024-10-18.at.8.29.04.PM.mov

@facebook-github-bot 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 Oct 19, 2024
@fangpenlin
Copy link
Author

Added a test sample file. I tested it in an Android emulator and it appears to be working fine. I haven't test other stuff or platform yet. Here's a screencast of the demo:

Screen.Recording.2024-10-18.at.8.29.04.PM.mov

@fangpenlin
Copy link
Author

Okay, tested with iOS simulator. It appears to be working as expected

Screen.Recording.2024-10-24.at.1.11.30.PM.mov

@fangpenlin fangpenlin force-pushed the bugfix-18945-inverted-sticky-footer branch from d77d594 to d121241 Compare October 27, 2024 04:17
@fangpenlin fangpenlin marked this pull request as ready for review October 28, 2024 15:08
@fangpenlin fangpenlin changed the title WIP: Fix #18945 inverted sticky headers (footers actually) not working issue Fix #18945 inverted sticky headers (footers actually) not working issue Oct 28, 2024
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 28, 2024
@fangpenlin
Copy link
Author

looks like there are some tests failed, will fix them shortly

@fangpenlin fangpenlin force-pushed the bugfix-18945-inverted-sticky-footer branch from d121241 to d5249a5 Compare October 31, 2024 04:58
@fangpenlin
Copy link
Author

Hey @migueldaipre, @cortinico and @cipolleschi, I've fixed the failing tests. Can you please help me kick start the CI job again as it appears requiring to be kicked started by repo maintainers.

@fangpenlin
Copy link
Author

hmm, okay, looks like some other tests are still failling 😅
will find a time to fix those soon

@fangpenlin fangpenlin force-pushed the bugfix-18945-inverted-sticky-footer branch from 01d4cfc to 593585a Compare November 2, 2024 04:05
@fangpenlin
Copy link
Author

Hi @migueldaipre, @cortinico and @cipolleschi, sorry about that. I updated the branch and ran test locally and it seems to be passing now. As for failing linter action, I didn't see anything from the messages look like causing by my changes. Is it suppose to fail regardless? If so, can you please help me trigger the CI action again? Thank you so much 🙏

@fangpenlin fangpenlin force-pushed the bugfix-18945-inverted-sticky-footer branch from 593585a to 85fda2b Compare November 5, 2024 02:37
@fangpenlin
Copy link
Author

I just realized that the linter failure is not caused by linter, it was actually prettier changes a few lines 😅
I have already run prettier and commit the changes. Should have all the CI action passes at this point

/cc
@migueldaipre, @cortinico and @cipolleschi

@fangpenlin
Copy link
Author

Somehow after rebased tests and linter failed again 😅
Sorry about that, let me check, rebased, and run them locally again to see if they pass or not

@fangpenlin
Copy link
Author

Okay, the flow error should be addressed now. Really sorry for repeating this many times 🙈
Hopefully this time there won't be other CI task failed, I run the known failed ones locally and they all seems like passing now. Finger cross 🤞

@migueldaipre, @cortinico and @cipolleschi

@cortinico
Copy link
Contributor

Hopefully this time there won't be other CI task failed, I run the known failed ones locally and they all seems like passing now. Finger cross 🤞

The CI is still red though 🤔

@cortinico
Copy link
Contributor

How about your work on your fork till you get the CI from GitHub actions on your fork green and then get back to the PR?

@fangpenlin fangpenlin force-pushed the bugfix-18945-inverted-sticky-footer branch from 85fda2b to 49d1bd6 Compare November 7, 2024 19:09
@fangpenlin fangpenlin force-pushed the bugfix-18945-inverted-sticky-footer branch from 49d1bd6 to fe7a407 Compare November 7, 2024 19:10
@fangpenlin
Copy link
Author

@cortinico sorry, I forgot to pushed my fixes for broken CI😅
Can you please help me run it on the CI one final time, if it still somehow fails, I will then setup GitHub action in my forked repo. Really sorry for the troubles 🙇‍♀️

@cortinico
Copy link
Contributor

Really sorry for the troubles 🙇‍♀️

No need to be sorry 🙏

Also just a small heads up: I'll be off for some weeks and I wasn't able to find a reviewer for this change internally, so this might have to wait some time before we can import/merge it

@fangpenlin
Copy link
Author

Haha, a bit embarrassing that the CI kept failing 😅
Looks like they all passed finally.

No worries, please enjoy your time off, and take your time. 🙏

@facebook-github-bot
Copy link
Contributor

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

@ACHP
Copy link

ACHP commented May 12, 2025

I hate to be that guy but … is there anything we can do to make this happens ? 😬 @fangpenlin

@cipolleschi
Copy link
Contributor

Sorry, I haven't had the time to work on this.
This change is also a bit delicate as it requires us to change some of the internal code and it will affect other Meta apps, so it is not just as easy as importing and landing.

@ACHP
Copy link

ACHP commented May 16, 2025

Ok I understand, thank you for your feedback 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SectionList][inverted] SectionList headers are sticky-at-the-top-footers if the list is inverted
5 participants