feat(ui): Add LOINC to sections & headers sticky#1314
Conversation
rogeruiz
requested review from
fzhao99,
jakewheeler,
lina-roth and
robertmitchellv
as code owners
rogeruiz
added
the
ux/ui
rogeruiz
force-pushed
the
rogeruiz/1277/section-table-improvements
branch
from
bafb0a8 to
28665eb
Compare
🔒 Security Scan Results
|
| Severity | Total |
|---|---|
| 🟠 High | 10 |
| 🟡 Medium | 4 |
📦 refiner-app
✅ No vulnerabilities found
📦 refiner-lambda
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
📦 refiner-ops
| Severity | Count |
|---|---|
| 🟠 High | 9 |
| 🟡 Medium | 2 |
View detailed results: Security tab
Last updated: 2026-06-05 20:32:30 UTC
|
I'm seeing double scroll bars. I'm thinking we may be missing a fixed height somewhere. Looks like maybe line 64? |
| currentSection={section} | ||
| sections={sectionProcessing} | ||
| disabled={disabled || isDisabledSection(section.code)} | ||
| <div className="max-h-[calc(100vh-400px)] overflow-y-auto"> |
There was a problem hiding this comment.
Doing things this way doesn't use the full height at smaller screen sizes.
Screen.Recording.2026-06-03.at.3.28.35.PM.mov
An alternative is to remove the div and instead make the individual th elements sticky and set the relative positioning using something like -top-6 z-10 bg-white
|
The tooltips seem to cut off. Maybe a z-index issue? |
This fixes the double scrollbar and the tooltip overflow in the eICR sections table. To do this, I had to split the eICR sections table into two separate header and body tables to avoid the clipping issue. The body table also now uses flex layout to prevent it from filling the parent container's remaining space. This removes the double scrollbar. Co-Authored-By: Jake Wheeler <jake.wheeler@skylight.digital> Co-Authored-By: fzhao99 <bob@skylight.digital>
|
Thanks @fzhao99 and @jakewheeler for your reviews. I think I've addressed |
| <RefineSwitch | ||
| </table> | ||
| <div className="min-h-0 flex-1 overflow-y-scroll"> | ||
| <table className="w-full table-fixed"> |
There was a problem hiding this comment.
breaking things into two tables like this will cause accessibility issues, since the bottom table doesn't have any semantic labeling and will get lost for screenreader users without associated header columns.
Is there a concern for keeping things in one semantic table and using sticky here like we were previously? Otherwise, this will come back to bite us when we need to submit accessibility scans at the next CDC review
There was a problem hiding this comment.
Yeah fair. I did think about this but wanted to see if this tradeoff would work well. We shouldn't degrade a11y for the sake of design. I have another option that should be more accessible using CSS grid or possibly using a different technique to render a tooltip. The breaking apart was done to allow the tooltip to escape the overflow-hidden on the table.
As @fzhao99 helpfully brought up, I was potentially sacrificing accessibility in order to avoid using Portals to render the tooltips. This refactor fixes any potential a11y issues with the Sections table and now uses Portals to render the tooltips in the `<body/>` tag rather than relative to where they appear in the DOM. Co-Authored-By: fzhao99 <bob@skylight.digital>
|
@fzhao99 this is ready for review again. |
|
@fzhao99 this should be in a good spot to review again. thanks |
Merge remote-tracking branch 'origin/main' into rogeruiz/1277/section-table-improvements
rogeruiz
force-pushed
the
rogeruiz/1277/section-table-improvements
branch
from
4aad65e to
e4fe0dd
Compare
3 participants
🔀 PULL REQUEST
💡 Summary
Makes the table headers for sections sticky by adding a scrollable wrapper. Also
the section code display is now showing on all sections not just custom ones.
🔗 Related Issue
✅ Acceptance Criteria
row
🧪 How to test
Load up the application locally. Navigate to a configuration's Sections view and
scroll to observe the sticky headers and the LOINC codes should now be visible
for all sections not just custom ones.