Skip to content

add css rule to keep header visible when maxHeight is set #3166

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.

Sign up for GitHub

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 6 commits into from
Jun 6, 2022
Merged

add css rule to keep header visible when maxHeight is set #3166

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
643645f
add css rule to keep header visible when maxHeight is set
May 25, 2022
b05573e
Merge branch 'main' into make-async-tableheader-visible
snowystinger May 26, 2022
e57d3ed
add css rule to keep header visible when maxHeight is set
May 25, 2022
b7226c1
Merge branch 'make-async-tableheader-visible' of github.com:ihgrant/r…
May 27, 2022
fa9c068
undo change in requestAnimationFrame mock
May 27, 2022
2b924be
Merge branch 'main' into make-async-tableheader-visible
LFDanLu Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@adobe/spectrum-css-temp/components/table/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ svg.spectrum-Table-sortedIcon {
border-left-style: solid;
border-right-width: 1px;
border-right-style: solid;
flex: 0 0 auto;
}
.spectrum-Table-headCellContents {
display: inline-block;
Expand Down
4 changes: 4 additions & 0 deletions packages/@react-spectrum/table/chromatic/TableView.chromatic.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,10 @@ export const NestedColumns = Template().bind({});
NestedColumns.storyName = 'nested columns';
NestedColumns.args = {columns: nestedColumns, items};

export const MaxHeight = Template.bind({});
Copy link
Member

@snowystinger snowystinger May 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ihgrant ihgrant May 26, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member

@snowystinger snowystinger May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ihgrant ihgrant May 27, 2022

Choose a reason for hiding this comment

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

Got the Chromatic test working using a literal component which demonstrates the bug if the CSS line is commented out. Thanks for the pointers!

MaxHeight.storyName = 'maxHeight defined';
MaxHeight.args = {columns, items, maxHeight: 'size-100'};

export const Empty = EmptyTemplate().bind({});
Empty.storyName = 'empty table';
Empty.args = {};
5 changes: 4 additions & 1 deletion packages/@react-spectrum/table/test/Table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ describe('TableView', function () {
});

beforeEach(() => {
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => {
Copy link
Member

@snowystinger snowystinger May 26, 2022

Choose a reason for hiding this comment

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

can we revert this change?

Copy link
Contributor Author

@ihgrant ihgrant May 26, 2022

Choose a reason for hiding this comment

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

Yeah, will do.

cb(0);
return 0;
});
});

afterEach(() => {
Expand Down