From 09970b81f9a8aad0b47f2574e476fc9ef3505223 Mon Sep 17 00:00:00 2001 From: Clay Miller Date: Fri, 18 Jun 2021 14:35:33 -0400 Subject: [PATCH 1/6] fix: Separate 'Item' content into 'label' and 'description' --- src/ActionList/Item.tsx | 69 +++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 37 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 46d9dd521c4..141c37a5500 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -151,9 +151,13 @@ const getItemVariant = (variant = 'default', disabled?: boolean) => { } } -const StyledItemContent = styled.div` +const StyledItemContent = styled.div<{ + descriptionVariant: ItemProps['descriptionVariant'] +}>` + align-items: baseline; display: flex; min-width: 0; + flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')}; flex-grow: 1; position: relative; ` @@ -229,16 +233,9 @@ const StyledItem = styled.div< ${sx} ` -export const TextContainer = styled.div<{ +export const TextContainer = styled.span<{ dangerouslySetInnerHtml?: React.DOMAttributes['dangerouslySetInnerHTML'] - descriptionVariant: ItemProps['descriptionVariant'] -}>` - display: flex; - min-width: 0; - flex-grow: 1; - flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')}; - align-items: baseline; -` +}>`` const BaseVisualContainer = styled.div<{variant?: ItemProps['variant']; disabled?: boolean}>` // Match visual height to adjacent text line height. @@ -310,7 +307,7 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El onKeyPress, children, onClick, - id, + id = uniqueId(), ...props } = itemProps @@ -358,6 +355,8 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El variant={variant} showDivider={showDivider} aria-selected={selected} + aria-labelledby={text ? `${id}-label` : undefined} + aria-describedby={text ? `${id}-description` : undefined} {...props} data-id={id} onKeyPress={keyPressHandler} @@ -393,35 +392,31 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El )} - + {children} - {(text || description) && ( - - {text &&
{text}
} - {description && ( - - {descriptionVariant === 'block' ? ( - description - ) : ( - - {description} - - )} - + {text ? {text} : null} + {description ? ( + + {descriptionVariant === 'block' ? ( + description + ) : ( + + {description} + )} - - )} - {(TrailingIcon || trailingText) && ( - - {trailingText &&
{trailingText}
} - {TrailingIcon && ( -
- -
- )} - - )} + + ) : null} + {(TrailingIcon || trailingText) && ( + + {trailingText &&
{trailingText}
} + {TrailingIcon && ( +
+ +
+ )} + + )} ) } From 0229d93f79d379dc6ca6115549a3039576e84bbf Mon Sep 17 00:00:00 2001 From: Clay Miller Date: Fri, 18 Jun 2021 15:40:36 -0400 Subject: [PATCH 2/6] fix: Only add 'aria-describedby' when 'description' exists --- src/ActionList/Item.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 141c37a5500..4a7b2c20e1c 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -356,7 +356,7 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El showDivider={showDivider} aria-selected={selected} aria-labelledby={text ? `${id}-label` : undefined} - aria-describedby={text ? `${id}-description` : undefined} + aria-describedby={description ? `${id}-description` : undefined} {...props} data-id={id} onKeyPress={keyPressHandler} From f38b82854dc5ac2753bc7e5410dcad600b2b39f5 Mon Sep 17 00:00:00 2001 From: Clay Miller Date: Fri, 18 Jun 2021 16:20:36 -0400 Subject: [PATCH 3/6] fix: Memoize 'id' so 'Item's and labels match --- src/ActionList/Item.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 4a7b2c20e1c..d3d8797f3c9 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -1,5 +1,5 @@ import {CheckIcon, IconProps} from '@primer/octicons-react' -import React, {useCallback} from 'react' +import React, {useCallback, useMemo} from 'react' import {get} from '../constants' import sx, {SxProp} from '../sx' import Truncate from '../Truncate' @@ -307,10 +307,12 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El onKeyPress, children, onClick, - id = uniqueId(), + id: _id, ...props } = itemProps + const id = useMemo(() => _id ?? uniqueId(), [_id]) + const keyPressHandler = useCallback( event => { if (disabled) { From 254a4a3a7092f00b1eafce450a508b6b758006d8 Mon Sep 17 00:00:00 2001 From: Clay Miller Date: Thu, 24 Jun 2021 16:33:56 -0400 Subject: [PATCH 4/6] =?UTF-8?q?fix:=20Don=E2=80=99t=20rely=20on=20'id'=20w?= =?UTF-8?q?hich=20is=20possibly=20not=20globally-unique?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/ActionList/Item.tsx | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index d3d8797f3c9..02ee7ca8c5e 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -307,11 +307,12 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El onKeyPress, children, onClick, - id: _id, + id, ...props } = itemProps - const id = useMemo(() => _id ?? uniqueId(), [_id]) + const labelId = useMemo(() => uniqueId(), []) + const descriptionId = useMemo(() => uniqueId(), []) const keyPressHandler = useCallback( event => { @@ -357,8 +358,8 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El variant={variant} showDivider={showDivider} aria-selected={selected} - aria-labelledby={text ? `${id}-label` : undefined} - aria-describedby={description ? `${id}-description` : undefined} + aria-labelledby={text ? labelId : undefined} + aria-describedby={description ? descriptionId : undefined} {...props} data-id={id} onKeyPress={keyPressHandler} @@ -396,9 +397,9 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El )} {children} - {text ? {text} : null} + {text ? {text} : null} {description ? ( - + {descriptionVariant === 'block' ? ( description ) : ( From 408ff2473c40a867b41c8bbedc1cf9499fc677ae Mon Sep 17 00:00:00 2001 From: Clay Miller Date: Fri, 25 Jun 2021 09:23:24 -0400 Subject: [PATCH 5/6] fix: Restore semi-full-width 'Item' dividers, without giving up the semantic nesting. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By “semantic nesting”, I mean that the 'Item' label and description are now siblings, which is preferable to the previous implementation, where the description node was a child of the label node. As a general principle, we should align DOM hierarchies with information hierarchies. An analogy: If I were using a bulleted list to describe a dog, I would not indent its breed as a second-level bullet beneath the bullet for its name, because a dog’s breed is not dependent/derived data from its name. Similarly, description is not dependent/derived from label, and so should not be nested in DOM. --- src/ActionList/Item.tsx | 89 ++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 42 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 02ee7ca8c5e..b3eeb9badf3 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -151,7 +151,16 @@ const getItemVariant = (variant = 'default', disabled?: boolean) => { } } -const StyledItemContent = styled.div<{ +const DividedContent = styled.div` + display: flex; + min-width: 0; + + /* Required for dividers */ + position: relative; + flex-grow: 1; +` + +const MainContent = styled.div<{ descriptionVariant: ItemProps['descriptionVariant'] }>` align-items: baseline; @@ -159,7 +168,6 @@ const StyledItemContent = styled.div<{ min-width: 0; flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')}; flex-grow: 1; - position: relative; ` const StyledItem = styled.div< @@ -192,7 +200,7 @@ const StyledItem = styled.div< :not(:first-of-type):not(${StyledDivider} + &):not(${StyledHeader} + &) { margin-top: ${({showDivider}) => (showDivider ? `1px` : '0')}; - ${StyledItemContent}::before { + ${DividedContent}::before { content: ' '; display: block; position: absolute; @@ -206,19 +214,19 @@ const StyledItem = styled.div< // Item dividers should not be visible: // - above Hovered - &:hover ${StyledItemContent}::before, + &:hover ${DividedContent}::before, // - below Hovered // '*' instead of '&' because '&' maps to separate class names depending on 'variant' - :hover + * ${StyledItemContent}::before, + :hover + * ${DividedContent}::before, // - above Focused - &:focus ${StyledItemContent}::before, + &:focus ${DividedContent}::before, // - below Focused // '*' instead of '&' because '&' maps to separate class names depending on 'variant' - :focus + * ${StyledItemContent}::before, + :focus + * ${DividedContent}::before, // - above Active Descendent - &.${itemActiveDescendantClass} ${StyledItemContent}::before, + &.${itemActiveDescendantClass} ${DividedContent}::before, // - below Active Descendent - .${itemActiveDescendantClass} + & ${StyledItemContent}::before { + .${itemActiveDescendantClass} + & ${DividedContent}::before { // '!important' because all the ':not's above give higher specificity border-color: transparent !important; } @@ -243,10 +251,6 @@ const BaseVisualContainer = styled.div<{variant?: ItemProps['variant']; disabled // hardcoded '20px' with '${get('space.s20')}'. height: 20px; width: ${get('space.3')}; - flex-shrink: 0; - display: flex; - flex-direction: column; - justify-content: center; margin-right: ${get('space.2')}; ` @@ -257,18 +261,21 @@ const ColoredVisualContainer = styled(BaseVisualContainer)` } ` -const LeadingVisualContainer = styled(ColoredVisualContainer)`` +const LeadingVisualContainer = styled(ColoredVisualContainer)` + flex-shrink: 0; + display: flex; + flex-direction: column; + justify-content: center; +` -const TrailingVisualContainer = styled(ColoredVisualContainer)` +const TrailingContent = styled(ColoredVisualContainer)` color: ${({variant, disabled}) => getItemVariant(variant, disabled).annotationColor}}; margin-left: ${get('space.2')}; margin-right: 0; + width: auto; div:nth-child(2) { margin-left: ${get('space.2')}; } - display: flex; - flex-direction: row; - justify-content: flex-end; ` const DescriptionContainer = styled.span<{descriptionVariant: ItemProps['descriptionVariant']}>` @@ -395,31 +402,29 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El )} - - {children} - {text ? {text} : null} - {description ? ( - - {descriptionVariant === 'block' ? ( - description - ) : ( - - {description} - - )} - + + + {children} + {text ? {text} : null} + {description ? ( + + {descriptionVariant === 'block' ? ( + description + ) : ( + + {description} + + )} + + ) : null} + + {TrailingIcon || trailingText ? ( + + {trailingText} + {TrailingIcon && } + ) : null} - - {(TrailingIcon || trailingText) && ( - - {trailingText &&
{trailingText}
} - {TrailingIcon && ( -
- -
- )} - - )} + ) } From 73eed06a2f81cd18e657f0a0c15f62cbde924555 Mon Sep 17 00:00:00 2001 From: Clay Miller Date: Fri, 25 Jun 2021 16:22:33 -0400 Subject: [PATCH 6/6] fix: Reduce styled-components class permutations. https://www.joshwcomeau.com/css/styled-components/ --- src/ActionList/Item.tsx | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index b3eeb9badf3..667683ce97c 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -160,13 +160,11 @@ const DividedContent = styled.div` flex-grow: 1; ` -const MainContent = styled.div<{ - descriptionVariant: ItemProps['descriptionVariant'] -}>` +const MainContent = styled.div` align-items: baseline; display: flex; min-width: 0; - flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')}; + flex-direction: var(--main-content-flex-direction); flex-grow: 1; ` @@ -278,16 +276,16 @@ const TrailingContent = styled(ColoredVisualContainer)` } ` -const DescriptionContainer = styled.span<{descriptionVariant: ItemProps['descriptionVariant']}>` +const DescriptionContainer = styled.span` color: ${get('colors.text.secondary')}; font-size: ${get('fontSizes.0')}; // TODO: When rem-based spacing on a 4px scale lands, replace // hardcoded '16px' with '${get('lh-12')}'. line-height: 16px; - margin-left: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? get('space.2') : 0)}; + margin-left: var(--description-container-margin-left); min-width: 0; flex-grow: 1; - flex-basis: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 0 : 'auto')}; + flex-basis: var(--description-container-flex-basis); ` const MultiSelectInput = styled.input` @@ -403,11 +401,23 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El )} - + {children} {text ? {text} : null} {description ? ( - + {descriptionVariant === 'block' ? ( description ) : (