New component: Toolbar #412

Nuhvi · 2021-01-30T18:46:31Z

Disclaimer: I did not communicate my intention to work on this so I will understand if you decided not to spend any time reviewing it, but I will appreciate it if you did!

I saw that Toolbar was not assigned to anyone in the Roadmap and I liked the documentation and example in WAI-ARIA

The reason I opened a draft PR is to make sure I am not going in the wrong direction, and ask what further requirements do you expect from this component before it is ready?

Thanks for your time.

Note: the Menu will not behave as shown in the screenshot above because #411

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code.
Add or edit tests to reflect the change (run yarn test).
Add or edit Storybook examples to reflect the change (run yarn dev).
Add documentation to support any new features.

This pull request:

Fixes a bug in an existing package
Adds additional features/functionality to an existing package
Updates documentation or example code
Other

…d-toolbar

* Add ToolbarGroup * Add ToolbarLabel * Add ToolbarSeparator * Add ToolbarItem * Use Orientation for styling Group and Separator * Create a story with ToggleButton, Group, DropDownMenu, and anchor link

benoitgrelard · 2021-02-01T10:22:46Z

This is awesome @nazeh! Thanks so much for this ♥️
We'll definitely take time to review it!

Nuhvi · 2021-02-01T17:07:03Z

Used composeEventHandlers to allow ToolbarItems to actually do stuff 🤦‍♂️

jjenzz

Please don't be discouraged by the amount of comments I've left here 😅 This PR is really great!! Thank you!!

Most of the stuff below is just little niggles but I love how well you've followed the overall architecture of our components 😍 It was a joy to review.

packages/react/toolbar/package.json

.yarn/versions/480f5556.yml

packages/react/toolbar/src/Toolbar.stories.tsx

packages/react/toolbar/src/Toolbar.tsx

benoitgrelard

@jjenzz's already outlined most of the feedback, I've added a couple more tidbits for consistency.

I just wanted to also say amazing work on that one following the current project structure, etc ♥️

packages/react/toolbar/package.json

packages/react/toolbar/src/Toolbar.tsx

…d-toolbar

* Use patch instead of prerelease * Update package.json scripts and version * Used *Class naming for css() * Remove ToolbarLabel * Add uniqueInputID * Remove pointerEvents: 'none' to better test onClick() * Use 'div' instead of 'button' to test 'disabled' * Update ToolbarItem onClick() to preventDefault() and skip itemProps.onClick for disabled itmes * Refactor onKeyDown() for the link in Toolbar.stories.tsx * Add onClick() instead of onKeyDown() for 'button' * Add onClick() for disabled ToolbarItem to make sure it does not trigger * Use Separator from react-separator in ToolbarSeparator * Use RavingFocuseGroupProps properly * Add defaults for 'dir' and 'loop' to match type defenition * Remove children if it is already spreaded * Inline React.uesMemo() * Use correct order of spreading props * Remove all use of any

Nuhvi · 2021-02-04T18:18:07Z

@jjenzz @benoitgrelard Thank you so much for the review and encouragement.
I made most of the requested changes, I think, except for two unresolved conversations about aria-control and the onClick in ToolbarItem, looking forward to your comments there.

I should be able to work on those in the morning, have a good afternoon!

packages/react/toolbar/src/Toolbar.tsx

jjenzz · 2021-02-08T19:12:54Z

This is looking great. Thanks for the updates @nazeh. I've left a few comments in response to bits and pieces above and a small change here #412 (comment)

Would be good to get @benoitgrelard's input on some of it. Super close now I think 👍

…d-toolbar

benoitgrelard · 2021-02-09T20:33:39Z

This is really close! ♥️
I've added my 2 cents on the last remaining (unresolved) bits (I've unresolved an extra one).

Nuhvi · 2021-02-09T21:01:21Z

Thanks @benoitgrelard,and @jjenzz for the feedback, I guess there are two more issues:

1- should I worry about adding Chromatic? and if so, how should I do that?

2- I notice a significant offset between the DropdownMenu and its trigger, do you have any idea what could be going wrong here before I start chasing my tail?

packages/react/toolbar/src/Toolbar.tsx

jjenzz · 2021-02-10T12:04:06Z

@nazeh , noooo, thank you for all your effort and patience with this 🙂

1- should I worry about adding Chromatic? and if so, how should I do that?

Since you won't have access to our Github Secrets, I will change the base branch here to toolbar-test and then once merged, I will create a new PR from there into main. That should make Chromatic run a baseline build of the Toolbar for us 🤞

2- I notice a significant offset between the DropdownMenu and its trigger, do you have any idea what could be going wrong here before I start chasing my tail?

Interesting, I'll take a look into this now quickly for you!

benoitgrelard · 2021-02-10T12:51:27Z

Regarding that gap in the dropdown menu, we're aware of this, it will happen in very specific circumstances when margins collapse on body. There are easy workarounds and we will looks at potentially replacing some dependency with our own to avoid it alltogether.

Thanks so much for your work on this @nazeh!

* New component: Toolbar (#412) * Add `ToolbarLink`, `ToolbarRadioGroup` & rename `ToolbarItem` * PR feedback * Forward `as` prop in `ToolbarRadioItem` * Replace inline component in `ToolbarRadioItem` with `Slot` * Update yarn.lock * Add chromatic stories * Add prominent toggle button toggled styles Co-authored-by: Ar Nazeh <[email protected]> Co-authored-by: Benoît Grélard <[email protected]>

Nuhvi added 4 commits January 25, 2021 06:07

[Toolbar][WIP] add new component

50f2bd6

Merge branch 'main' of https://github.com/radix-ui/primitives into ad…

729adc6

…d-toolbar

Merge branch 'main' of https://github.com/radix-ui/primitives into ad…

94108ab

…d-toolbar

[toolbar] Update with basic features

2f68be6

* Add ToolbarGroup * Add ToolbarLabel * Add ToolbarSeparator * Add ToolbarItem * Use Orientation for styling Group and Separator * Create a story with ToggleButton, Group, DropDownMenu, and anchor link

Nuhvi requested review from benoitgrelard and jjenzz as code owners January 30, 2021 18:46

Add a release strategy

206fe83

Nuhvi mentioned this pull request Jan 31, 2021

Children are omitted from MenuImpl in #409, how can I add MenuItems? #411

Closed

Nuhvi added 2 commits February 1, 2021 19:21

Merge branch 'main' into add-toolbar

efd67d0

[toolbar][fix] compose toolbarItem and RovingFocus eventHandlers

d7f9345

jjenzz suggested changes Feb 3, 2021

View reviewed changes

benoitgrelard reviewed Feb 3, 2021

View reviewed changes

packages/react/toolbar/package.json Outdated Show resolved Hide resolved

packages/react/toolbar/src/Toolbar.tsx Outdated Show resolved Hide resolved

packages/react/toolbar/src/Toolbar.tsx Outdated Show resolved Hide resolved

Nuhvi added 2 commits February 4, 2021 18:20

Merge branch 'main' of https://github.com/radix-ui/primitives into ad…

a9cc921

…d-toolbar

jjenzz reviewed Feb 8, 2021

View reviewed changes

packages/react/toolbar/src/Toolbar.tsx Outdated Show resolved Hide resolved

packages/react/toolbar/src/Toolbar.tsx Outdated Show resolved Hide resolved

jjenzz changed the title ~~Draft: New component: Toolbar~~ New component: Toolbar Feb 8, 2021

Nuhvi added 2 commits February 9, 2021 10:39

Merge branch 'main' of https://github.com/radix-ui/primitives into ad…

20a3e97

…d-toolbar

Remove TOOLBAR_DEFUALT_TAG

ab99020

Remove onClick() and rename SeparatorImpl

8fb36f5

benoitgrelard reviewed Feb 9, 2021

View reviewed changes

packages/react/toolbar/src/Toolbar.tsx Outdated Show resolved Hide resolved

Fix type ToolbarSeparatorPrimitive

a974f1a

jjenzz changed the base branch from main to toolbar-test February 10, 2021 12:05

jjenzz approved these changes Feb 10, 2021

View reviewed changes

jjenzz merged commit df0cf48 into radix-ui:toolbar-test Feb 10, 2021

benoitgrelard pushed a commit that referenced this pull request Feb 15, 2021

New component: Toolbar (#412)

2a6e091

jjenzz pushed a commit that referenced this pull request Feb 16, 2021

New component: Toolbar (#412)

48f8ad0

New component: Toolbar #412

New component: Toolbar #412

Uh oh!

Conversation

Nuhvi commented Jan 30, 2021

Uh oh!

benoitgrelard commented Feb 1, 2021

Uh oh!

Nuhvi commented Feb 1, 2021

Uh oh!

jjenzz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benoitgrelard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nuhvi commented Feb 4, 2021

Uh oh!

Uh oh!

Uh oh!

jjenzz commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benoitgrelard commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nuhvi commented Feb 9, 2021

Uh oh!

Uh oh!

jjenzz commented Feb 10, 2021

Uh oh!

benoitgrelard commented Feb 10, 2021

Uh oh!

Uh oh!

jjenzz left a comment •

edited

Loading

jjenzz commented Feb 8, 2021 •

edited

Loading

benoitgrelard commented Feb 9, 2021 •

edited

Loading