feat: Add height="initial" to Overlay
#1287
Conversation
|
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/55Q36wPoRwzpsCEqR25BWvcR25CW |
smockle
changed the title
height="initial" to Overlay
src/Overlay.tsx
Outdated
| @@ -9,7 +9,7 @@ import {useCombinedRefs} from './hooks/useCombinedRefs' | |||
|
|
|||
| type StyledOverlayProps = { | |||
| width?: keyof typeof widthMap | |||
| height?: keyof typeof heightMap | |||
| height?: string | |||
There was a problem hiding this comment.
This could be switched back now that we have initial in the map, right?
There was a problem hiding this comment.
Yup, that change is no longer necessary—we can realign the internal (StyledOverlayProps) and external (OverlapProps) contracts.
Incidentally, the reason isn’t because initial is included in heightMap, but rather because imperatively setting height means we are no longer passing arbitrary height values to the StyledOverlay component, so the type of height no longer needs to be widened from keyof typeof heightMap to string.
There was a problem hiding this comment.
Sidebar: Do you think CSP would block the imperative approach? If so, we could restore the original approach (in eb298ad).
…d external contract; 'height' is not widened to 'string' in the former, because arbitrary pixel values are no longer passed via props.
Partial-fix for https://github.com/github/primer/issues/161. Specifically, this fixes the case where
SelectPanelitems are available initially (i.e. not asynchronously fetched).This PR adds an
"initial"value toOverlay’sheightprop, which cause the component height to fit contents initially (like"auto"), then be fixed at that initial height, so that as contents change (e.g. by filtering), the container remains at the initial height (unlike, and preferred to,"auto").Sidebar: The asynchronously-fetch case is partially-supported: A component consumer can initially (before items have been retrieved) pass
height="auto"then (once items are retrieved) passheight="initial". BecauseheightKeyis a dependency of theuseEffect, the height-after-fetch can be set using this approach.Screen recordings
Overflowing items
The following recording demonstrates that an
Overlaywithheight="initial"initially opens shorter than its contents (which are scrollable, because they do not fit within theOverlay’smax-height), and remains at this height as contents are filtered. This is functionally identical toheight="XSmall"when items overflow.Underflowing items
The following recording demonstrates that an
Overlaywithheight="initial"initially opens exactly as large as its contents (which are not scrollable, because they fit within theOverlay’smax-height), and remains at this height as contents are filtered. This is superior to bothheight="XSmall"(which, with this few items, would undesirably include empty space underneath them when theOverlayopens) andheight="auto"(which would remove empty space after filtering, undesirably varying theOverlayheight).Merge checklist