Skip to content
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

fix(recordings): fix recording filters from being cutoff in nested archived recordings tables #557

Merged
merged 9 commits into from
Oct 18, 2022

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Oct 17, 2022

Fixes #556
Depends on #555

@maxcao13 maxcao13 added the fix label Oct 17, 2022
@maxcao13
Copy link
Member Author

maxcao13 commented Oct 17, 2022

Currently investigating how to fix the tests since I added a prop menuAppendTo="parent" instead of "inline" to the Select component inside the []Filter components which appends the select dropdowns to the "parent" component. But the test renderer for the snapshot tests doesn't seem to have a parent.

@maxcao13 maxcao13 requested review from tthvo and andrewazores and removed request for tthvo October 17, 2022 23:10
@tthvo
Copy link
Member

tthvo commented Oct 18, 2022

Nicee catch! Could you also change the menuAppendTo of DatetimePicker to parent too?

const elementToAppend = document.body;

Look like the toolbar now is in a constrainted container (i.e. recording-table-container) in previous commits now allow parent instead of document.body. This way we can avoid accessibility issue: https://www.patternfly.org/v4/components/select/#appending-document-body-vs-parent

@tthvo
Copy link
Member

tthvo commented Oct 18, 2022

Also, on this topic, this issue seems to be caused by the table having sticky header as referenced here. Now that all recording tables are wrapped in OuterScrollContainer and InnerScrollContainer, should we make all recording tables "sticky" too?

isStickyHeader={props.isNestedTable}

Then, all filters must be checked to avoid being cut-off.

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks good! Just another notice about the filter selects:

Screenshot from 2022-10-18 03-23-44

Looks like the select menu is not scrollable and could go as long as it needs. I think we should allow scrolling. This could be done by setting maxHeight on Select (Reference):

<Select maxHeight={"300px"}>
  {options}
</Select>

What do you think?

@andrewazores
Copy link
Member

andrewazores commented Oct 18, 2022

If that maxHeight prop supports general CSS size units then it would be better to use a relative size (like em or vh) rather than absolute (like px) so that this better supports displays with different sizes and pixel densities.

@andrewazores
Copy link
Member

While you're at that, please update these two px uses as well:

src/app/SecurityPanel/Credentials/MatchedTargetsTable.tsx
126:        <InnerScrollContainer style={{ height: '300px' }}>

src/app/app.css
81:  height: 512px; 

@github-actions
Copy link

This PR/issue depends on:

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks nicee! Fixed the bug :D

@andrewazores andrewazores merged commit e41e2c8 into cryostatio:main Oct 18, 2022
mergify bot pushed a commit that referenced this pull request Oct 18, 2022
…chived recordings tables (#557)

* fixed issues

* just add an optional menuAppendTo option in []Filter components and set to default inline in the snapshot tests

* prettier

* changed z-index of recording table so that filter dropdown is always above, and changed the recordings headers to always be sticky, fixed tests

* prettier

* adjusted px heights to relative heights

* change filter select size 10em -> 16em

* prettier

* fixed max-height to height for recording table container

(cherry picked from commit e41e2c8)
andrewazores pushed a commit that referenced this pull request Oct 18, 2022
…chived recordings tables (#557) (#562)

* fixed issues

* just add an optional menuAppendTo option in []Filter components and set to default inline in the snapshot tests

* prettier

* changed z-index of recording table so that filter dropdown is always above, and changed the recordings headers to always be sticky, fixed tests

* prettier

* adjusted px heights to relative heights

* change filter select size 10em -> 16em

* prettier

* fixed max-height to height for recording table container

(cherry picked from commit e41e2c8)

Co-authored-by: Max Cao <macao@redhat.com>
@maxcao13 maxcao13 deleted the filter-bug-fix branch October 18, 2022 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Recording filters are cutoff in nested Archived recording tables
3 participants