-
Notifications
You must be signed in to change notification settings - Fork 556
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
Introduce TrailingAction to ActionList #4634
Changes from 78 commits
7f21a16
fa708b1
e087f87
76e2606
eab6fd9
8b4f311
614749a
bfb8fe2
4b18eed
d13668d
10d9866
0939a90
cc6a63c
1cd9fb1
3d1daf3
5999a77
9f77b09
087ab1f
f4a5021
ba31e40
8e2921d
afa8d9f
4b1f2b4
10cd551
65c23c8
70659ed
1a9b3e0
1f783fa
6774f93
5ed547f
cf5ecf9
2f13c76
f32f416
84f041e
55632b1
8bc5c15
ce70779
3e8af88
4c8a4de
0d076dc
d2faa9a
54eab93
d1c1d60
c4fb363
8b70194
da5b5a8
6d0bc54
da464e9
950ac4a
1db6a02
b20fc5e
87bfa62
9ad7d86
c218363
dd91f97
dc216ad
001fcf8
a69a44c
35579ab
c67b0e1
66e5405
96a9ef3
6011c9b
5028660
0bc749c
804153b
0152e99
c238a6b
d0355a2
8802b79
3362ce5
676b276
ea88abe
088c981
294f4d2
4667520
e9147b0
3058906
29fac6a
a84033c
8a9b2d4
5539174
ec25af5
b1f65d6
9d53def
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@primer/react": minor | ||
--- | ||
|
||
Introduce ActionList.TrailingAction to support secondary action on ActionList.Item |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,9 @@ import {Selection} from './Selection' | |
import {getVariantStyles, ItemContext, TEXT_ROW_HEIGHT, ListContext} from './shared' | ||
import type {VisualProps} from './Visuals' | ||
import {LeadingVisual, TrailingVisual} from './Visuals' | ||
import {TrailingAction} from './TrailingAction' | ||
import {ConditionalWrapper} from '../internal/components/ConditionalWrapper' | ||
import {invariant} from '../utils/invariant' | ||
import {useFeatureFlag} from '../FeatureFlags' | ||
|
||
const LiBox = styled.li<SxProp>(sx) | ||
|
@@ -71,6 +73,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
const [slots, childrenWithoutSlots] = useSlots(props.children, { | ||
leadingVisual: LeadingVisual, | ||
trailingVisual: TrailingVisual, | ||
trailingAction: TrailingAction, | ||
blockDescription: [Description, props => props.variant === 'block'], | ||
inlineDescription: [Description, props => props.variant !== 'block'], | ||
}) | ||
|
@@ -125,6 +128,10 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
|
||
const itemRole = role || inferredItemRole | ||
|
||
if (slots.trailingAction) { | ||
invariant(!container, `ActionList.TrailingAction can not be used within a ${container}.`) | ||
} | ||
|
||
/** Infer the proper selection attribute based on the item's role */ | ||
let inferredSelectionAttribute: 'aria-selected' | 'aria-checked' | undefined | ||
if (itemRole === 'menuitemradio' || itemRole === 'menuitemcheckbox') inferredSelectionAttribute = 'aria-checked' | ||
|
@@ -134,6 +141,9 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
|
||
const {theme} = useTheme() | ||
|
||
// Ensures ActionList.Item retains list item semantics if a valid ARIA role is applied, or if item is inactive | ||
const listSemantics = listRole === 'listbox' || listRole === 'menu' || inactive || container === 'NavList' | ||
|
||
const activeStyles = { | ||
fontWeight: 'bold', | ||
bg: 'actionListItem.default.selectedBg', | ||
|
@@ -149,10 +159,32 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
}, | ||
} | ||
|
||
const hoverStyles = { | ||
'@media (hover: hover) and (pointer: fine)': { | ||
':hover:not([aria-disabled]):not([data-inactive])': { | ||
backgroundColor: `actionListItem.${variant}.hoverBg`, | ||
color: getVariantStyles(variant, disabled, inactive).hoverColor, | ||
boxShadow: `inset 0 0 0 max(1px, 0.0625rem) ${theme?.colors.actionListItem.default.activeBorder}`, | ||
}, | ||
'&:focus-visible, > a.focus-visible, &:focus.focus-visible': { | ||
outline: 'none', | ||
border: `2 solid`, | ||
boxShadow: `0 0 0 2px ${theme?.colors.accent.emphasis}`, | ||
}, | ||
':active:not([aria-disabled]):not([data-inactive])': { | ||
backgroundColor: `actionListItem.${variant}.activeBg`, | ||
color: getVariantStyles(variant, disabled, inactive).hoverColor, | ||
}, | ||
}, | ||
} | ||
|
||
const listItemStyles = { | ||
display: 'flex', | ||
// show between 2 items | ||
':not(:first-of-type)': {'--divider-color': theme?.colors.actionListItem.inlineDivider}, | ||
width: 'calc(100% - 16px)', | ||
marginX: buttonSemantics ? '2' : '0', | ||
...(buttonSemantics ? hoverStyles : {}), | ||
Comment on lines
+185
to
+187
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. @TylerJDev, do we still need this? 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. Kinda, this gives consistency between the link items and the regular items when you hover over the |
||
} | ||
|
||
const styles = { | ||
|
@@ -163,7 +195,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
paddingY: '6px', // custom value off the scale | ||
lineHeight: TEXT_ROW_HEIGHT, | ||
minHeight: 5, | ||
marginX: listVariant === 'inset' ? 2 : 0, | ||
marginX: listVariant === 'inset' && !buttonSemantics ? 2 : 0, | ||
borderRadius: 2, | ||
transition: 'background 33.333ms linear', | ||
color: getVariantStyles(variant, disabled, inactive).color, | ||
|
@@ -181,28 +213,11 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
appearance: 'none', | ||
background: 'unset', | ||
border: 'unset', | ||
width: listVariant === 'inset' ? 'calc(100% - 16px)' : '100%', | ||
width: listVariant === 'inset' && !buttonSemantics ? 'calc(100% - 16px)' : '100%', | ||
fontFamily: 'unset', | ||
textAlign: 'unset', | ||
marginY: 'unset', | ||
|
||
'@media (hover: hover) and (pointer: fine)': { | ||
':hover:not([aria-disabled]):not([data-inactive])': { | ||
backgroundColor: `actionListItem.${variant}.hoverBg`, | ||
color: getVariantStyles(variant, disabled, inactive).hoverColor, | ||
boxShadow: `inset 0 0 0 max(1px, 0.0625rem) ${theme?.colors.actionListItem.default.activeBorder}`, | ||
}, | ||
'&:focus-visible, > a.focus-visible, &:focus.focus-visible': { | ||
outline: 'none', | ||
border: `2 solid`, | ||
boxShadow: `0 0 0 2px ${theme?.colors.accent.emphasis}`, | ||
}, | ||
':active:not([aria-disabled]):not([data-inactive])': { | ||
backgroundColor: `actionListItem.${variant}.activeBg`, | ||
color: getVariantStyles(variant, disabled, inactive).hoverColor, | ||
}, | ||
}, | ||
|
||
'@media (forced-colors: active)': { | ||
':focus, &:focus-visible, > a.focus-visible': { | ||
// Support for Windows high contrast https://sarahmhigley.com/writing/whcm-quick-tips | ||
|
@@ -224,6 +239,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
borderTopWidth: showDividers ? `1px` : '0', | ||
borderColor: 'var(--divider-color, transparent)', | ||
}, | ||
|
||
// show between 2 items | ||
':not(:first-of-type)': {'--divider-color': theme?.colors.actionListItem.inlineDivider}, | ||
// hide divider after dividers & group header, with higher importance! | ||
|
@@ -237,6 +253,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
'--divider-color': 'transparent', | ||
}, | ||
...(active ? activeStyles : {}), | ||
...(listSemantics || _PrivateItemWrapper ? hoverStyles : {}), | ||
khiga8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
const clickHandler = React.useCallback( | ||
|
@@ -268,8 +285,6 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
const inlineDescriptionId = `${itemId}--inline-description` | ||
const blockDescriptionId = `${itemId}--block-description` | ||
const inactiveWarningId = inactive && !showInactiveIndicator ? `${itemId}--warning-message` : undefined | ||
// Ensures ActionList.Item retains list item semantics if a valid ARIA role is applied, or if item is inactive | ||
const listSemantics = listRole === 'listbox' || listRole === 'menu' || inactive || container === 'NavList' | ||
|
||
const ButtonItemWrapper = React.forwardRef(({as: Component = 'button', children, ...props}, forwardedRef) => { | ||
return ( | ||
|
@@ -424,6 +439,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |
{slots.blockDescription} | ||
</Box> | ||
</ItemWrapper> | ||
{!inactive && Boolean(slots.trailingAction) && !container && slots.trailingAction} | ||
</LiBox> | ||
</ItemContext.Provider> | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import React, {forwardRef} from 'react' | ||
import Box from '../Box' | ||
import {Button, IconButton} from '../Button' | ||
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' | ||
|
||
export type ActionListTrailingActionProps = { | ||
as?: 'button' | 'a' | ||
href?: string | ||
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. What would you think about defining types like this? {
as?: 'button'
href: never // href is invalid for button type
} |
{
as?: 'a'
href: 'string' // href is required for a type
} I have seen we did this before and I thought it is cool! 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. OOH TIL 😁 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. ok this didn't work for me and I haven't quite figure out the proper syntax 😅 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. @broccolinisoup, I continued to poke around with the syntax. See 194f847. I looked at other examples in the code and did not see any examples that achieve what we want. 🤔 I would be open any ideas you have to get the types to work, but I don't think this should block us from moving forward! 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. @khiga8 are the semantics of this something like this: type ElementProps =
| {
as?: 'button'
href?: never
}
| {
as: 'a'
href: string
}
export type ActionListTrailingActionProps = ElementProps & {
icon?: React.ElementType
label: string
} Or is that incorrect / not working due to the issue you talked about above with |
||
icon?: React.ElementType | ||
label: string | ||
showOnHover?: boolean | ||
} | ||
|
||
export const TrailingAction = forwardRef( | ||
({as = 'button', icon, label, showOnHover = false, href = null}, forwardedRef) => { | ||
if (!icon) { | ||
return ( | ||
<Box | ||
data-component="ActionList.TrailingAction" | ||
as="span" | ||
sx={{ | ||
flexShrink: 0, | ||
visibility: showOnHover ? 'hidden' : 'visible', | ||
}} | ||
> | ||
{/* @ts-expect-error TODO: Fix this */} | ||
<Button variant="invisible" as={as} href={href} ref={forwardedRef}> | ||
{label} | ||
</Button> | ||
</Box> | ||
) | ||
} else { | ||
return ( | ||
<Box | ||
as="span" | ||
data-component="ActionList.TrailingAction" | ||
sx={{ | ||
flexShrink: 0, | ||
visibility: showOnHover ? 'hidden' : 'visible', | ||
}} | ||
> | ||
<IconButton | ||
as={as} | ||
aria-label={label} | ||
icon={icon} | ||
variant="invisible" | ||
unsafeDisableTooltip={false} | ||
tooltipDirection="w" | ||
href={href} | ||
// @ts-expect-error StyledButton wants both Anchor and Button refs | ||
ref={forwardedRef} | ||
/> | ||
</Box> | ||
) | ||
} | ||
}, | ||
) as PolymorphicForwardRefComponent<'button' | 'a', ActionListTrailingActionProps> | ||
|
||
TrailingAction.displayName = 'ActionList.TrailingAction' |
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.
Just realized block variant is supported for Description so added this
ActionList.Description variant="block"
example!@langermank Is the Trailing Action alignment okay as is, or should it be vertically centered?
From storybook draft:
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.
For reference, this is with a "Trailing Visual":
It's similar with the "inactive" button. If we should modify this instance, do we need to adjust the others?