LG-5027: Toolbar #2809
LG-5027: Toolbar #2809
Conversation
* rm create-leafygreen-app * Update README.md * lint
🦋 Changeset detectedLatest commit: a9c9c31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 34 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +70 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
* upate hooks * with changeset * revert allowProp
* update webpack * findStories args * changeset * Update index.ts * revert to getLGConfig
| /** | ||
| * The LG Icon that will render in the button | ||
| */ | ||
| glyph: GlyphName; |
There was a problem hiding this comment.
I like this approach better but wonder if we should also build this directly into IconButton. This seems like a better experience for consumers and not sure why we'd need to diverge with 2 different methods
<IconButton>
<IconName />
</IconButton>
<ToolbarIconButton glyph="IconName" />
There was a problem hiding this comment.
This might be because you can create a custom glyph component , which you can then pass as a child to IconButton. With ToolbarIconButton, you can only use our glyphs.
There was a problem hiding this comment.
oh I see, I wasn't aware we allow that for IconButton
| /** | ||
| * The text that will render in the tooltip on hover | ||
| */ | ||
| label: React.ReactNode; |
There was a problem hiding this comment.
do we need to be opinionated about requiring a label? could consumers instead rely on an aria-label without rendering a tooltip?
There was a problem hiding this comment.
I confirmed with Sandy that Tooltips are required.
| /** | ||
| * LGIDs for Toolbar components. | ||
| */ | ||
| lgids: GetLgIdsReturnType; |
| &:hover, | ||
| &[data-hover='true'], | ||
| &::before { | ||
| border-radius: 6px; |
There was a problem hiding this comment.
| border-radius: 6px; | |
| border-radius: ${borderRadius[150]}px; |
| background: ${theme === Theme.Light | ||
| ? palette.green.light3 | ||
| : palette.green.dark3}; |
There was a problem hiding this comment.
We don't have component-specific tokens as yet. Were you thinking that we should already have tokens for these?
The closest token background tokens we have are for a success state, but I wouldn't consider this a success state.
https://github.com/mongodb/leafygreen-ui/blob/main/packages/tokens/src/color/darkModeColors.ts#L39
https://github.com/mongodb/leafygreen-ui/blob/main/packages/tokens/src/color/lightModeColors.ts#L39
There was a problem hiding this comment.
ah these were the ones I was thinking of, but you're right that these aren't quite right for success state. I guess I'm wondering if we should carve out a separate semantic token for this and the color. looks similar to what's used for active menu items
There was a problem hiding this comment.
If the tokens involve updating other components, I'd rather we do that work in a separate PR.
| trigger={ | ||
| <div className={triggerStyles}> | ||
| <IconButton | ||
| aria-label={getNodeTextContent(label)} | ||
| active={active} | ||
| className={getIconButtonStyles({ | ||
| theme, | ||
| active, | ||
| disabled, | ||
| className, | ||
| })} | ||
| tabIndex={isFocusable ? 0 : -1} | ||
| onClick={handleOnClick} | ||
| disabled={disabled} | ||
| data-testid={lgids?.iconButton} | ||
| data-lgid={lgids?.iconButton} | ||
| data-active={active} | ||
| ref={ref} | ||
| {...(rest as ComponentPropsWithoutRef<'button'>)} | ||
| > | ||
| <Icon glyph={glyph} /> | ||
| </IconButton> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
From the figma, it looks like the left/right borders are included in the 48x48 and that is implemented correctly. However, it looks like the 48x48 should also include the top border which doesn't seem to be included currently. Can that be double-checked?
| figma | storybook |
|---|---|
 |
 |
There was a problem hiding this comment.
Confirmed with Sandy that the engineering implementation is fine.
| trigger={ | ||
| <div className={triggerStyles}> | ||
| <IconButton | ||
| aria-label={getNodeTextContent(label)} |
There was a problem hiding this comment.
if we always render a tooltip, should we instead define an aria-labelledby?
There was a problem hiding this comment.
whats the advantage of this?
There was a problem hiding this comment.
From reading up on the MDN docs on aria-label and aria-labelledby, I was thinking that aria-labelledby made more sense thinking that the label could be referenced, but I forget that it's not actually visible in the DOM until triggered
However, a separate thing I'm wondering now is: does this label provide proper context about what this IconButton does? I seem to recall that design wanted the label text to be short, but was that only for the design where the text was rendered vertically below the icon? What do you think providing some contextual details about the label? Probably something like Go to ${label} or Open drawer for ${label}
If consumers don't like the interpolated string, we can treat it as a default value and allow them to specify their own aria-label
aria-label={ariaLabelProp || `Go to ${getNodeTextContent(label)}`}
There was a problem hiding this comment.
@sandynguyenn what do you think about this?
There was a problem hiding this comment.
Spoke with Sandy, and shes ok with providing a default and letting people customize it
There was a problem hiding this comment.
This is a little tricky. Toolbar should not be linked to Drawer, since it can be used anywhere, and we won't always know what the action will be. It can open a Drawer, open a Modal, or some other action. This is why we don't control the active state. Therefore, I think that we should not add more content, and if the consumer wants context, they can provide it themselves.
The G Suite toolbar also features extremely short labels.
There was a problem hiding this comment.
Gotcha, this might be a case where we want good guidelines around providing enough context for screen readers. G Suite has short one-word labels for specific Google products while also using more detailed labels for non-product cases like Report Phish and Get Add-ons
I think it would be helpful to consumers if our examples took a similar approach, and we do what we can to ensure consumers set these up properly given there is no visible label
There was a problem hiding this comment.
sg, i'll update the examples
| data-testid={lgids?.tooltip} | ||
| data-lgid={lgids?.tooltip} |
There was a problem hiding this comment.
These should be unique if we want to enable targeting specific ToolbarIconButton instances. We can do so by destructuring id from the useDescendant call and interpolating it here
| data-testid={lgids?.iconButton} | ||
| data-lgid={lgids?.iconButton} |
There was a problem hiding this comment.
These should be unique if we want to enable targeting specific ToolbarIconButton instances. We can do so by destructuring id from the useDescendant call and interpolating it here
There was a problem hiding this comment.
The consumer can get a specific instance if they use the test harness, but I also can't think of any negative side effects of making the ids unique as well. We usually keep them the same for dropdown options, but this component serves a different purpose. I can update these.
| const handleOnClick = (event: React.MouseEvent<HTMLButtonElement>) => { | ||
| onClick?.(event); | ||
| // This ensures that on click, the buttons tabIndex is set to 0 so that when the up/down arrows are pressed, the correct button is focused | ||
| setFocusedIndex?.(index); | ||
| }; |
There was a problem hiding this comment.
can this be defined once in Toolbar instead as opposed to in each separate ToolbarIconButton instance?
There was a problem hiding this comment.
Do you mean create the function in Toolbar and pass it to the context?
There was a problem hiding this comment.
yes, so it's not recreated for each ToolBarIconButton instance
| cx( | ||
| css` | ||
| height: 100%; | ||
| width: ${spacing[1200]}px; |
There was a problem hiding this comment.
similar note to icon button height: I think it's better to avoid using spacing tokens for height/width and instead define a separate constant for magic numbers like this. e.g. const TOOLBAR_WIDTH = 48;
| // Show the focus ring on the toolbar itself when a child element is focused and only when navigating with a keyboard | ||
| &:has(:focus-visible) { | ||
| box-shadow: ${focusRing[theme].default}; | ||
| } |
There was a problem hiding this comment.
was this a specific design request? it looks strange to me to have the focus ring apply to the full toolbar
There was a problem hiding this comment.
I'm following this accessibility pattern
|
|
||
| const Template: StoryFn<typeof Toolbar> = props => <Toolbar {...props} />; | ||
|
|
||
| export const LiveExample = Template.bind({}); |
There was a problem hiding this comment.
wondering if it makes sense to enable controlling the active ToolbarIconButton instance in the live example? it's difficult to test all the interactions without that
There was a problem hiding this comment.
What interaction would you test if there were an active control? Or do you want the ability to click on an icon and set that icon to active?
There was a problem hiding this comment.
yes being able to change the active instance seems useful for the live example story
There was a problem hiding this comment.
Since this isn't a controlled component, I'm hesitant to add the ability to change the active state, as it might give the consumer the impression that this component is controlled. SideNav is a similar component that has an active state, but we don't have an option to control it in the story.
I also found an example from the telerik design system, and they don't update the active state in the live example.
However, we could add an example updating the active state in a codesandbox.
| import { getTestUtils, Toolbar, ToolbarIconButton, ToolbarProps } from '.'; | ||
|
|
||
| export default { | ||
| title: 'Components/Toolbar/Ineractions', |
There was a problem hiding this comment.
| title: 'Components/Toolbar/Ineractions', | |
| title: 'Components/Toolbar/Interactions', |
| ); | ||
| const [focusedIndex, setFocusedIndex] = useState(0); | ||
| const childrenLength = descendants?.length ?? 0; | ||
| const [isUsingKeyboard, setIsUsingKeyboard] = useState(false); |
There was a problem hiding this comment.
should we use the existing UsingKeyboardContext in place of this state?
There was a problem hiding this comment.
I tried that, but the initial value for UsingKeyboardContext is true, which means it was causing this component to hijack the focus.
| const ids = { | ||
| root, | ||
| iconButton: `${DEFAULT_LGID_ROOT}-icon_button`, | ||
| tooltip: `${DEFAULT_LGID_ROOT}-tooltip`, |
There was a problem hiding this comment.
wondering if this might be more explicit as iconButtonTooltip. something like:
| tooltip: `${DEFAULT_LGID_ROOT}-tooltip`, | |
| iconButtonTooltip: `${DEFAULT_LGID_ROOT}-icon_button-tooltip`, |
packages/toolbar/src/index.ts
Outdated
| export { | ||
| DEFAULT_LGID_ROOT, | ||
| getLgIds, | ||
| type GetLgIdsReturnType, | ||
| getTestUtils, | ||
| type TestUtilsReturnType, | ||
| } from './utils'; |
There was a problem hiding this comment.
pending the status of the new */testing package subdirectory approach, can we start using that here or add a ticket to refactor this before shipping v1?
packages/toolbar/README.md
Outdated
| ### Usage | ||
|
|
||
| ```tsx | ||
| import { getTestUtils } from '@leafygreen-ui/gallery-indicator'; |
There was a problem hiding this comment.
| import { getTestUtils } from '@leafygreen-ui/gallery-indicator'; | |
| import { getTestUtils } from '@leafygreen-ui/toolbar'; |
I also wonder if we can start using the new */testing package subdir approach or mark a todo with ticket to refactor these when that approach is vetted and ready to use
There was a problem hiding this comment.
I can create a ticket for it. We should wait until it's ready to use.
There was a problem hiding this comment.
Actually, creating the testing directory should be doable
packages/toolbar/README.md
Outdated
| #### Props | ||
|
|
||
| | Prop | Type | Description | Default | | ||
| | ---------- | ----------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------- | | ||
| | `active` | `boolean` | Determines if the icon button is in an active state | `false` | | ||
| | `glyph` | `Glyph` | Name of the icon glyph to display in the button. List of available glyphs can be found in the [Icon README](https://github.com/mongodb/leafygreen-ui/blob/main/packages/icon/README.md#properties). | | | ||
| | `label` | `string` | Text that appears in the tooltip on hover/focus | | | ||
| | `onClick` | `(event: React.MouseEvent) => void` | Callback fired when the button is clicked | | | ||
| | `disabled` | `boolean` | Determines if the icon is disabled | `false` | |
There was a problem hiding this comment.
what do you think about listing the explicit props that are added in ToolbarIconButton and linking to the IconButton read me for other props? Also can add a note about which props are omitted from that set
I worry about docs getting out of sync if IconButton props change because it requires that the future eng who works on IconButton changes also know that they need to make updates here. I'm flexible on the final approach
* updates lg install script * updates meta * adds --dry option to install command * changesets * adds package set flags * cli minor * fix install pkg prebuild * adds tests * updates lint scrips * lint all_packages file on write * run lint * Update getLGConfig.ts * Update getPackagesToInstall.ts * Update getPackagesToInstall.spec.ts * fix mocks * update flags to be additive * push not concat * Update getLGConfig.spec.ts * Update package.json * adds separate files for ESSENTIALS & BASIC * --ui * readonly<> * fix tests * Update pnpm-lock.yaml * add missing lodash
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| @@ -0,0 +1,7 @@ | |||
| export { getTestUtils, type TestUtilsReturnType } from './testing'; | |||
There was a problem hiding this comment.
awesome, thanks for using the new /testing entry point. we should be able to drop this from this index.ts file now
There was a problem hiding this comment.
I don't think we should drop this as yet, since the entry points work is still being worked on
| ToolbarIconButton, | ||
| type ToolbarIconButtonProps, | ||
| } from './ToolbarIconButton'; | ||
| export { DEFAULT_LGID_ROOT, getLgIds, type GetLgIdsReturnType } from './utils'; |
There was a problem hiding this comment.
similarly, I don't think we need to include these in the root package exports
| ### Usage | ||
|
|
||
| ```tsx | ||
| import { getTestUtils } from '@leafygreen-ui/toolbar'; |
There was a problem hiding this comment.
| import { getTestUtils } from '@leafygreen-ui/toolbar'; | |
| import { getTestUtils } from '@leafygreen-ui/toolbar/testing'; |
There was a problem hiding this comment.
The entry points PR has not been published, so I don't think we should update this as yet.
| ```tsx | ||
| import { render } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import { Toolbar, ToolbarIconButton, getTestUtils } from '@leafygreen-ui/toolbar'; |
There was a problem hiding this comment.
| import { Toolbar, ToolbarIconButton, getTestUtils } from '@leafygreen-ui/toolbar'; | |
| import { Toolbar, ToolbarIconButton } from '@leafygreen-ui/toolbar'; | |
| import { getTestUtils } from '@leafygreen-ui/toolbar/testing'; |
| ```tsx | ||
| import { render } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import { Toolbar, ToolbarIconButton, getTestUtils } from '@leafygreen-ui/toolbar'; |
There was a problem hiding this comment.
| import { Toolbar, ToolbarIconButton, getTestUtils } from '@leafygreen-ui/toolbar'; | |
| import { Toolbar, ToolbarIconButton } from '@leafygreen-ui/toolbar'; | |
| import { getTestUtils } from '@leafygreen-ui/toolbar/testing'; |
✍️ Proposed changes
🎟 Jira ticket: LG-5027
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changesFor new components
🧪 How to test changes