[Layout] Rebuild OptionList with layout primitives
#7958
Conversation
size-limit report 📦
|
aveline
requested review from
alex-page,
laurkim and
kyledurand
and removed request for
alex-page
There was a problem hiding this comment.
Tophatted all the stories for OptionList at the breakpoints locally and all except the one with sections look visually the same, but I know you mentioned the extra spacing in the description 👍
It's probably worth confirming with @sarahill or @nayeobkim that the extra 4px is okay before merging this to the batch 3 branch.
| role={role} | ||
| > | ||
| {/* eslint-disable-next-line @typescript-eslint/ban-ts-comment */} | ||
| {/* @ts-ignore OptionList allows string as role but Box doesn't. @TODO: remove string from OptionList in v11 */} |
There was a problem hiding this comment.
👍 Let's just make sure this is added to the v11 road map / issue / PR, whatever we have going for that
Another thing I think you could do is cast role as BoxProps['role'] or maybe as unknown as BoxProps['role'] and that way you wouldn't need 4 lines of comments. I'm fine with leaving it as is, maybe uglier is better for visibility
There was a problem hiding this comment.
There's a branch for v11 #7597 so I'll go ahead and open a pr against it.
| <ul className={styles.OptionList} role={role}> | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore OptionList allows string as role but Box doesn't. @TODO: remove string from OptionList in v11 | ||
| <Box as="ul" role={role} padding="2"> |
There was a problem hiding this comment.
It seems like this is where the added space is coming from. Can we use isFirstOption here to change the value like you did above?
There was a problem hiding this comment.
The added spacing only happens on sections where there's no title, that's the combination where there were overlapping margins.
I added a Bleed to handle that and resolve the change in spacing. It feels hacky but at least it prevents the visual regression.
| <Box | ||
| as="ul" | ||
| id={`${id}-${sectionIndex}`} | ||
| role={role as BoxProps['role']} |
There was a problem hiding this comment.
I think the uglier version with all the comments is better for visibility. But we can ship either one.
There was a problem hiding this comment.
I do agree the one with all the comments was better for visibility to know to remove it but I'm okay with either version!
WHY are these changes introduced?
Fixes #7952
WHAT is this pull request doing?
Updates
OptionListto use new layout primitives.One small visual change with this update due to replacing
marginwithpaddingis a slight increase (4px) in spacing when there are multiple sections in anOptionList. I don't think the added complexity to keep this the same as current doesn't seem worth it, but happy to hear other thoughts.Update: Added a
Bleedto resolve the spacing change. Feels hacky but prevents the visual change.