Skip to content

Sticky header using intersection observer for recordset page #2608

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

Merged
merged 24 commits into from
Jun 18, 2025

Conversation

KenyShah24
Copy link
Contributor

This PR includes code changes to ensure the table header remains sticky while scrolling content on the recordset page using the Intersection Observer.

Issues Identified During Testing:

  1. E2E Testing with Docker:
  • The Docker image build process encounters compatibility issues with Node.js and npm versions required for the Chaise project.
  • Additionally, openjdk-8-sdk is missing when running the script.
    The included changes address these dependency errors
  1. E2E Testing Locally:
  • On Windows, the mkdir -p command is not functioning as expected.
  • A solution for this issue is still being explored.

@KenyShah24 KenyShah24 requested a review from RFSH February 11, 2025 17:49
@RFSH RFSH self-assigned this Feb 11, 2025
Copy link
Member

@RFSH RFSH left a comment

Choose a reason for hiding this comment

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

The implemented solution is not responsive and therefore we cannot merge it as is.

The vertical position of the sticky header is calculated on the initial load and remains the same. So if the user squeezes the page or remove a banner/alert (which changes the height of the top section), the sticky header shows up in the wrong spot.

Also there's no reason to keep the height in the React state. It's only used for manipulating the position of sticky header and not used in the render logic, so we shouldn't add it to state.

Other changes that should be applied:

  • Please remove .last-run.json.
  • Revert the docker file changes that you did.
  • Revert the changes in locators/recordset.ts. Instead of using filter, we just need to change the selector to use the :not(.sticky-header) stynax. You can just revert you changes. Once we think this PR is ready, I'll take care of test cases myself.

Copy link
Member

@RFSH RFSH left a comment

Choose a reason for hiding this comment

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

We had a call about this and I'm going to summarize what we talked about here:

The attachContainerHeightSensors function already does the calculations that we need and so we shouldn't add another resizeSensor for doing similar calculations.

Instead we can add a callback as an argument to attachContainerHeightSensors which would tell us about the height changes. And then we can use that callback for setting the top of the sticky header.

Copy link
Member

@RFSH RFSH left a comment

Choose a reason for hiding this comment

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

With the latest changes, the sticky header is now properly adjusting as the available space above the table changes. The width of the headers are also adjusting properly.

But there are still two functionality issues that I mentioned in my comments and I'll summarize here:

  1. The top calculation is not working properly for the popups. It's not honoring the vertical space outside of the modal, so the sticky header shows up on top of the page size dropdown.

  2. This implementation is forcing React to rerender the recordset component and all its children after each height/width change. This can cause performance issues on busier pages.

By the way, please update this branch with the latest changes of the main branch.

@RFSH RFSH merged commit b60190b into master Jun 18, 2025
3 checks passed
@RFSH RFSH deleted the sticky-header-intersection-observer branch June 18, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants