add css rule to keep header visible when maxHeight is set #3166
Conversation
ihgrant
force-pushed
the
make-async-tableheader-visible
branch
from
d248a51 to
643645f
Compare
|
|
||
| beforeEach(() => { | ||
| jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb()); | ||
| jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => { |
There was a problem hiding this comment.
can we revert this change?
| NestedColumns.storyName = 'nested columns'; | ||
| NestedColumns.args = {columns: nestedColumns, items}; | ||
|
|
||
| export const MaxHeight = Template.bind({}); |
There was a problem hiding this comment.
So this actually doesn't render anything
you can run our chromatic storybook if you run the command yarn start:chromatic as defined in our package.json
That aside, I don't think we really need this, we'd really need to test it against the async version to meet your reproduction from what you've said. It also looks like the Template is not applying the maxHeight where you expect.
I think it's plenty this time to just contribute the fix.
There was a problem hiding this comment.
Okay, despite my initial report I think this bug occurs even with non-async lists, so I'll spend a bit of time to see if I can get this working. I wasn't aware I could run Chromatic locally without a token, that should help.
So this actually doesn't render anything
Is this because I am (mistakenly) not calling Template?
There was a problem hiding this comment.
Honestly, I didn't dig into why it wasn't rendering anything. If it happens in the non-async case, that'd be immensely helpful. Is it easy to reproduce?
I'd check the definition of Template a little further up in the file, looks like it already has a maxHeight on the TableView. You also don't need to use the Template, you can just return a component. So if you have a reproduction case, you could just copy it in and it should work.
If this is taking too much time, I'm really ok with just the change.
There was a problem hiding this comment.
Got the Chromatic test working using a literal component which demonstrates the bug if the CSS line is commented out. Thanks for the pointers!
…eact-spectrum into make-async-tableheader-visible
4 participants
Closes #3159
I added a Chromatic story to illustrate the fix - haven't used Chromatic or Storybook before so let me know if I did something wrong!
✅ Pull Request Checklist:
📝 Test Instructions:
packages/@react-spectrum/table/stories/Table.stories.tsxon line 157 to change theheightprop tomaxHeight.