Skip to content

Conversation

@didoo
Copy link
Contributor

@didoo didoo commented Apr 14, 2025

📌 Summary

Looking at #2789 I noticed a couple of possible improvements.

🛠️ Detailed description

In this PR I have:

  • moved the iFrame with the AdvancedTable AppFrame demo in the “AdvancedTable” showcase page
    • reason: is more consistent with the other "demo" sections, it provide an in-page example via the iFrame, and a link to the full page via the "external link" icon in the frame
  • moved the “overflow wrapping logic” from the AppFrame main container to a local wrapper of the GenericAdvancedTable demo component
    • reason: we're not touching the main container, making the example more similar to what consumers would have in production

⚠️ Notice: @shleewhite this made me think if the "problem" of having a wrapper with overflow: auto (or min-width: 0) would be a problem that consumer would have too. In which case, we should consider having the hds-advanced-table__container as display: grid or something similar, so this would work out of the box instead of having to taking care of the overflow themselves. @KristinLBradley what do you think?

Preview:


👀 Component checklist

  • Percy was checked for any visual regression

💬 Please consider using conventional comments when reviewing this PR.

@didoo didoo requested a review from a team as a code owner April 14, 2025 22:13
@vercel
Copy link

vercel bot commented Apr 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Apr 28, 2025 3:56pm
hds-website ✅ Ready (Inspect) Visit Preview Apr 28, 2025 3:56pm

@shleewhite
Copy link
Contributor

shleewhite commented Apr 15, 2025

@didoo agree about the overflow: auto thing.. this came up because the scroll bar was not showing up in the "right" context for the horizontal scrolling.

Another thing, we have a ticket open to investigate the best way to set the height for the table. In the showcase, I had to cap the table at 600px to make sure the thead would stick which isnt ideal. The overflow: auto fix could also be added in with that work.

shleewhite
shleewhite previously approved these changes Apr 15, 2025
Copy link
Contributor

@shleewhite shleewhite left a comment

Choose a reason for hiding this comment

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

Changes look good, I just left the one suggestion.

shleewhite
shleewhite previously approved these changes Apr 24, 2025
@didoo didoo force-pushed the advanced-table-app-frame-followup branch from 07eeb14 to 45319d2 Compare April 25, 2025 10:42
@didoo didoo requested a review from a team as a code owner April 25, 2025 10:42
@hashibot-hds hashibot-hds added packages/components docs-website Content updates to the documentation website labels Apr 25, 2025
@didoo didoo changed the base branch from main to hds-4777/revive-enterprise-nav April 25, 2025 10:42
@didoo
Copy link
Contributor Author

didoo commented Apr 25, 2025

@shleewhite I've rebased my branch on hds-4777/revive-enterprise-nav (#2837) to avoid conflicts in your PR (which is more urgent/important)

shleewhite
shleewhite previously approved these changes Apr 25, 2025
@shleewhite shleewhite force-pushed the hds-4777/revive-enterprise-nav branch from 68e6ec1 to 641c8ff Compare April 25, 2025 15:28
Base automatically changed from hds-4777/revive-enterprise-nav to main April 28, 2025 14:35
@shleewhite shleewhite dismissed their stale review April 28, 2025 14:35

The base branch was changed.

@didoo
Copy link
Contributor Author

didoo commented Apr 28, 2025

@LilithJames-HDS after your branch has been merged to main I had to rebase; can you re-approve?

@didoo didoo merged commit aae7aa3 into main Apr 28, 2025
16 checks passed
@didoo didoo deleted the advanced-table-app-frame-followup branch April 28, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants