Repository navigation
fix(library): let long shelf names breathe in the Move To menu #240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,7 +112,11 @@ export function Dropdown(props: DropdownProps): JSX.Element { | |
| ? { | ||
| top: menuPos.top, | ||
| left: menuPos.left, | ||
| width: menuPos.width, | ||
| // Never narrower than the trigger, never wider than the room | ||
| // left of the viewport's right edge, and capped so a long shelf | ||
| // name can't stretch the menu across the whole screen. | ||
| minWidth: menuPos.width, | ||
| maxWidth: `min(320px, calc(100vw - ${menuPos.left + 8}px))`, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two edge cases in this
Neither is likely with the current compact "Move To" pill in a centered modal, so this may not be worth guarding for now — flagging in case the |
||
| transform: | ||
| menuPos.placement === 'top' ? 'translateY(-100%)' : undefined, | ||
| } | ||
|
|
@@ -167,7 +171,12 @@ export function Dropdown(props: DropdownProps): JSX.Element { | |
| </div> | ||
| </> | ||
| ) : ( | ||
| <Text variant={TypographyVariant.TextBase}>{option.label}</Text> | ||
| <Text | ||
| variant={TypographyVariant.TextBase} | ||
| className={styles.optionLabel} | ||
| > | ||
| {option.label} | ||
| </Text> | ||
| )} | ||
| {hasSubOptions ? ( | ||
| <ArrowIcon | ||
|
|
@@ -198,7 +207,10 @@ export function Dropdown(props: DropdownProps): JSX.Element { | |
| onKeyDown={activateOnKey(() => handleSubSelect(sub.value))} | ||
| aria-label={`Select ${sub.label}`} | ||
| > | ||
| <Text variant={TypographyVariant.TextBase}> | ||
| <Text | ||
| variant={TypographyVariant.TextBase} | ||
| className={styles.optionLabel} | ||
| > | ||
| {sub.label} | ||
| </Text> | ||
| {value === sub.value && ( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overflow-wrap: anywhereis applied to.optionLabel, which is also used inside.subMenufor sub-option labels (Dropdown.tsx~L210-215). But.subMenusetswhite-space: nowrap(line 198), which is inherited by.optionLabeland takes precedence overoverflow-wrap—white-space: nowrapsuppresses all soft-wrap opportunities, including the emergency breaksoverflow-wrap: anywherewould otherwise insert. So a long, unbreakable sub-option label would still overflow the sub-menu rather than wrap.Not triggered by today's only sub-option consumer (
Shelf.tsx'sPrivate/Publicprivacy options are short), so this won't reproduce right now — but it means the fix is silently inert for sub-menus, which contradicts the stated intent ("Shelf and library names can be a single long token ... Break it instead") since sub-option labels can be arbitrary CMS-driven text too. Worth an explicitwhite-space: normal;(or removingnowrapfrom.subMenuand relying onmin-width: 100%+overflow-wrap) so this doesn't resurface as a real bug once a long sub-option label ships.Fix this →