-
Notifications
You must be signed in to change notification settings - Fork 28
Conditional sticky table head shadows #2010
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
Conversation
This should make writing play functions easier, since we can use the full testing-library query suite
| }) | ||
| global.MutationObserver = mutationObserverMock | ||
|
|
||
| const intersectionObserverMock = jest.fn().mockImplementation(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 2 locations. Consider refactoring.
…de-dvc into conditional-table-shadows
| const intersectionObserverMock = jest.fn().mockImplementation(() => { | ||
| return { | ||
| disconnect: jest.fn(), | ||
| observe: jest.fn(), | ||
| unobserve: jest.fn() | ||
| } | ||
| }) | ||
| global.IntersectionObserver = intersectionObserverMock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the job of keeping things from breaking and not much more. Could be made better if we wanted to actually puppet the observer, but I think we have better ways to test most scenarios it would apply to.
| type: MessageFromWebviewType.REORDER_COLUMNS | ||
| }) | ||
| } | ||
| const [ref, scrolled] = useInView({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[C] Would be good to have an indicator here to know this is for the sticky header shadow. [ref, headerNeedsShadow]? Just something for the next person that comes along. that doesn't involve them following it through into the scss file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first thought was scrolled because that's describing what the variable contains regardless of how we use it, but I can see how that would be helpful where it's only used in one place now.
|
Code Climate has analyzed commit 327e456 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.6% (0.0% change). View more on Code Climate. |
#2005 <- this
fixes #1826
This PR makes the shadow on the sticky table header only appear when the table is scrolled a bit (set as 15px, but can be changed to anything if we want via
rootMargin).conditional-header-shadow-demo.mp4
Opted to rely on Chromatic for testing, as the result is purely visual. To do this, I added a new "Scrolled" story to the table stories that uses a custom viewport and scrolls to the bottom-right in its
playfunction.