DEX-922 implement stickyHeader and add a meaningful maxHeight to ever… #430
Conversation
…y DataDisplay. Also add horizontalScroll
src/pages/fieldManagement/settings/defaultFieldComponents/SpeciesEditor.jsx
Outdated
Show resolved
Hide resolved
… Card component and replaced with optional styles, which was then implemented for ImageCard and StatusCard. Remove maxHeight from Card in relationshipCard, remove unused paperStyles from DataDisplay.
Responded in 1873372. |
| @@ -75,6 +75,7 @@ export default function IndividualsDisplay({ | |||
| data={individuals} | |||
| title={title} | |||
| loading={loading} | |||
| maxHeight={600} | |||
There was a problem hiding this comment.
Should there be a maxHeight on paginated tables. If we want the table to be shorter, shouldn't we just reduce the request page size?
There was a problem hiding this comment.
That's a good point. Are we convinced that every instantiation of IndividualsDisplay is going to be paginated?
There was a problem hiding this comment.
IndividualsDisplay passes its rest props to its DataDisplay component, so if an instance would rather have a maximum height than be paginated, it can pass maxHeight.
There was a problem hiding this comment.
Ok, I've noticed two weird things now:
-
When passing maxHeight to IndividualsDisplay, the DataDisplay does NOT seem to be respecting it:
<IndividualsDisplay
individuals={searchResults || []}
hideFilterSearch
loading={loading}
sortExternally
searchParams={searchParams}
setSearchParams={setSearchParams}
dataCount={resultCount}
style={{ maxHeight: 600 }}
/> -
The limit: 20 param in useFilterIndividuals.js also seems not to be respected. I created an investigation ticket for this latter one: DEX-1322.
There was a problem hiding this comment.
Removed in c1cb541. So, currently that means that this table is displaying a ton of rows.
There was a problem hiding this comment.
For the first weird thing: You are not technically passing maxHeight to IndividualsDisplay, you are passing style. Neither of those two props are used directly by IndividualsDisplay. Instead they are destructured into its rest property and spread as props to DataDisplay. DataDisplay does destructure maxHeight, but style is once again destructured into the rest property. The maxHeight prop is used in the TableContainer's style, but rest is spread to the div wrapping all of DataDisplay's other children. That div has the default overflow: visible, so it may not look like maxHeight is working, but it is.
When I pass style={{ maxHeight: 200, border: '1px solid red' }} to IndividualsDisplay, it is only 200px tall, but it overflows. Notice that the pagination is right below the 200px tall display:
When I pass maxHeight={200} to IndividualsDisplay, the table height is constrained:
There was a problem hiding this comment.
Maybe instead of adding a maxHeight prop to DataDisplay, we should add a tableContainerStyles prop instead?
Punted this to DEX-1321. |
Atticus29
deleted the
DEX-922-fixed-header-vertical-scroll-horizontal-scroll
branch
* Automatic Docker builds on tagged releases and PRs with multi-platform support * Remove nightly action, which was merged into build * Allow all files to be copied in * Dex 761 social group site settings (#419) * DEX-761 flesh out social group form except for multipleInGroup * DEX-761 begin fleshing out the switch * DEX-761 continue with fleshing out the switch * DEX-761 add useEffect * DEX-761 rename the role component, clean up, and remove the unnecessary useState * DEX-761 internationalize and add description as caption * DEX-761 respond to code review feedback * DEX-761 remove unecessary flexbox div * DEX-761 handle the case where a guidless role makes it into the SocialGroupRole component * move server status components to siteStatus directory * Change server status text to site status * Add server status SuperText keys, prevent 'hide' prop from passing to Text * Internationalize aria labels in site status components * dex 1117 - Replace progress with pipeline_status in Asset Group (#420) * Replace asset group progress with pipeline_status * Change px to number and abstract showAlert in AssetGroup * Add public AGS page (#416) * Add public AGS page * Remove unused property * Fixes * Rename things * Remove unused translation key * use new terms (#418) * begin release procedure (#424) * begin release procedure * clarify about merge conflicts * respond to feedback * respond to code review feedback * Update release procedure doc * Run on sentence Co-authored-by: Ben Scheiner <[email protected]> * DEX-922 implement stickyHeader and add a meaningful maxHeight to ever… (#430) * DEX-922 implement stickyHeader and add a meaningful maxHeight to every DataDisplay. Also add horizontalScroll * DEX-922 remove seemingly unnecessary horizontal scroll prop * DEX-922 respond to code review feedback. Remove default maxHeight for Card component and replaced with optional styles, which was then implemented for ImageCard and StatusCard. Remove maxHeight from Card in relationshipCard, remove unused paperStyles from DataDisplay. * DEX-922 respond to code review feedback pt 2 * DEX-922 add tableContainerStyles to DataDisplay and remove maxHeight * DEX-922 remove tableContainerStyles from SightingsDisplay and fix a few little things * DEX-922 final little fixes * Compute new form state from previous state in Settings (#433) * 922 follow up (#435) * DEX-922 print the collaboration data to make the stackBlitz question easier * DEX-922 I do not understand why tableData was undefined, but cool, codex, I can play this game with you * DEX-922 try disablePortal * DEX-922 change the z-index * DEX-922 clean up * DEX-922 respond to code review feedback * Update version to 1.0.2 (#436) Co-authored-by: Jason Parham <[email protected]> Co-authored-by: Mark <[email protected]> Co-authored-by: Ben Scheiner <[email protected]>
…y DataDisplay. Also add horizontalScroll
Pull request checklist:
developbranch, notmainWhich JIRA ticket(s) and/or GitHub issues does this PR address?
DEX-922
Thanks for keeping it clean! Feel free to merge your own pull request after it has been approved - just use the "squash & merge" option.
Implements a frozen column header for all tables that have a maxHeight. Implements said maxHeight for every current instance of DataDisplay.
Afaict, the horizontal scrolling behavior already works as desired, but if not, I implemented a default behavior for all DataDisplays explicitly in commit a43d331173be17076950881418300ea822cb6ebc, which I removed in the subsequent commit.
Note that I did not have a mouse available while implementing this, so I was unable to test this.
QA notes:
Populate several tables (IMHO doesn't have to be exhaustive to pass QA) with lots of values and confirm that the sticky header works in all observed cases. Repeat with and without a mouse. Repeat in a different browser.
Affected tables: