From bf779e2f49977a58c22a0b4797e88070fe79c327 Mon Sep 17 00:00:00 2001 From: Clay Miller Date: Fri, 25 Jun 2021 17:58:06 -0400 Subject: [PATCH] fix: Split `` labels (#1320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Separate 'Item' content into 'label' and 'description' * fix: Only add 'aria-describedby' when 'description' exists * fix: Memoize 'id' so 'Item's and labels match * fix: Don’t rely on 'id' which is possibly not globally-unique * fix: Restore semi-full-width 'Item' dividers, without giving up the semantic nesting. 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. * fix: Reduce styled-components class permutations. https://www.joshwcomeau.com/css/styled-components/ --- src/ActionList/Item.tsx | 134 ++++++++++++++++++++++------------------ 1 file changed, 74 insertions(+), 60 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index cfc2839c430..57d474d1fcb 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' @@ -13,6 +13,7 @@ import { activeDescendantActivatedIndirectly, isActiveDescendantAttribute } from '../behaviors/focusZone' +import {uniqueId} from '../utils/uniqueId' /** * These colors are not yet in our default theme. Need to remove this once they are added. @@ -153,11 +154,21 @@ const getItemVariant = (variant = 'default', disabled?: boolean) => { } } -const StyledItemContent = styled.div` +const DividedContent = styled.div` display: flex; min-width: 0; - flex-grow: 1; + + /* Required for dividers */ position: relative; + flex-grow: 1; +` + +const MainContent = styled.div` + align-items: baseline; + display: flex; + min-width: 0; + flex-direction: var(--main-content-flex-direction); + flex-grow: 1; ` const StyledItem = styled.div< @@ -193,7 +204,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; @@ -207,23 +218,23 @@ 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 { // allow override in case another item in the list is active/focused border-color: var(--item-hover-divider-border-color-override, transparent) !important; } // - 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 - &[${isActiveDescendantAttribute}] ${StyledItemContent}::before, + &[${isActiveDescendantAttribute}] ${DividedContent}::before, // - below Active Descendent - [${isActiveDescendantAttribute}] + & ${StyledItemContent}::before { + [${isActiveDescendantAttribute}] + & ${DividedContent}::before { // '!important' because all the ':not's above give higher specificity border-color: transparent !important; } @@ -248,16 +259,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. @@ -265,10 +269,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')}; ` @@ -279,30 +279,33 @@ 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']}>` +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` @@ -333,6 +336,9 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El ...props } = itemProps + const labelId = useMemo(() => uniqueId(), []) + const descriptionId = useMemo(() => uniqueId(), []) + const keyPressHandler = useCallback( event => { if (disabled) { @@ -377,6 +383,8 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El variant={variant} showDivider={showDivider} aria-selected={selected} + aria-labelledby={text ? labelId : undefined} + aria-describedby={description ? descriptionId : undefined} {...props} data-id={id} onKeyPress={keyPressHandler} @@ -412,35 +420,41 @@ export function Item(itemProps: Partial & {item?: ItemInput}): JSX.El )} - - {children} - {(text || description) && ( - - {text &&
{text}
} - {description && ( - - {descriptionVariant === 'block' ? ( - description - ) : ( - - {description} - - )} - - )} - - )} - {(TrailingIcon || trailingText) && ( - - {trailingText &&
{trailingText}
} - {TrailingIcon && ( -
- -
- )} - - )} - + + + {children} + {text ? {text} : null} + {description ? ( + + {descriptionVariant === 'block' ? ( + description + ) : ( + + {description} + + )} + + ) : null} + + {TrailingIcon || trailingText ? ( + + {trailingText} + {TrailingIcon && } + + ) : null} + ) }