Floating toolbar #1956
Floating toolbar #1956
Conversation
✅ Deploy Preview for modus-webcomponents ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
stencil-workspace/storybook/stories/components/modus-toolbar/modus-toolbar-storybook-docs.mdx
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
I wasn't able to review in storybook from the site in the PR, as it wasn't showing the menu. That will need to be looked into. I pulled down locally for review.
For the split and horizontal scenario, the toolbar is offset to the right a bit relative to the other examples
Also I noticed the rounded left border was missing when combined, both horizontal and vertical (image shows top as current state, bottom is with a corrected border)
Both the modus styleguide and figma have the divider color as --modus-gray-0 (#e0e1e9)
The background in the modus styleguide and figma are both white. Might be good to audit all the color choices.
There are other small details in the modus styleguide, such as the rounder borders on the button separate from the rounded borders of the toolbar.
Other than that, would be great if you could handle dark mode as well.
stencil-workspace/src/components/modus-toolbar/button/modus-toolbar-button.tsx
Outdated
Show resolved
Hide resolved
stencil-workspace/src/components/modus-toolbar/button/modus-toolbar-button.tsx
Outdated
Show resolved
Hide resolved
| disabled={this.disabled} | ||
| onClick={() => (!this.disabled ? this.buttonClick.emit() : null)}> | ||
| {this.iconSrc && <img src={this.iconSrc} />} | ||
| {this.textButton} |
There was a problem hiding this comment.
I'm not sure text as context is approved at the moment. Is this a required use case for your scenario? Both the modus styleguide and the figma don't include text examples.
There was a problem hiding this comment.
The text buttons were discussed on the issue created for this component (#1420) multiple use cases were provided and it was approved.
There was a problem hiding this comment.
@sorina-ocheana I apologize, I did not update myself with the submitted issue. The decisions made in the ticket were not reflected in the areas I was told were the sources of truth. I will review the issue and adjust any comments accordingly as well as work internally to ensure we have a pathway to updating our sources of truth after decisions are made.
| }) | ||
| export class ModusToolbar { | ||
| /** The buttons to render. */ | ||
| @Prop() buttons: ModusToolbarButton[] = []; |
There was a problem hiding this comment.
If this was an array of an array of buttons, then could we manage the divider in a more natural way? No property for divider anymore. If combined, then there will be a divider between groups. Perhaps a ModusToolbarButtonGroup array as the outer type.
There was a problem hiding this comment.
Discussed on chat.
✅ Deploy Preview for moduswebcomponents ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
| }; | ||
|
|
||
| export const VerticalSplitToolbar = Template.bind({}); | ||
| Default.args = { |
There was a problem hiding this comment.
This should set the args for VerticalSplitToolbar
| }, | ||
| }; | ||
|
|
||
| const Template = ({buttons, layout, toolbarStyle}) => html`<modus-toolbar buttons=${buttons} layout=${layout} toolbarStyle=${toolbarStyle}></modus-toolbar>`; |
There was a problem hiding this comment.
To use property binding you'll need to put a . in front of your attribute.
const Template = ({buttons, layout, toolbarStyle}) => html`
<modus-toolbar .buttons=${buttons} .layout=${layout} .toolbarStyle=${toolbarStyle}></modus-toolbar>
`;This should fix it not showing in the docs and canvas.
|
Make sure you run |
|
thanks for your work on this @sorina-ocheana - we have a (floating) Toolbar component added now! |
Description
Added the floating toolbar component
References #1420
Type of change
How Has This Been Tested?
I've added the component to the index.html using its HTML tag.
Checklist