Skip to content

Conversation

@marek-mihok
Copy link
Contributor

@marek-mihok marek-mihok commented Mar 27, 2023

This change brings the commands attribute for ui.button where you can provide other relevant actions for the button that are displayed in the context menu.

export interface Button {
  /** An identifying name for this component. If the name is prefixed with a '#', the button sets the location hash to the name when clicked. */
  name: Id
  /** The text displayed on the button. */
  label?: S
  /** The caption displayed below the label. */
  caption?: S
  /** A value for this button. If a value is set, it is used for the button's submitted instead of a boolean True. */
  value?: S
  /** True if the button should be rendered as the primary button in the set. */
  primary?: B
  /** True if the button should be disabled. */
  disabled?: B
  /** True if the button should be rendered as link text and not a standard button. */
  link?: B
  /** An optional icon to display next to the button label (not applicable for links). */
  icon?: S
  /** The width of the button, e.g. '100px'. */
  width?: S
  /** True if the component should be visible. Defaults to True. */
  visible?: B
  /** An optional tooltip message displayed when a user clicks the help icon to the right of the component. */
  tooltip?: S
  /** The path or URL to link to. If specified, the `name` is ignored. The URL is opened in a new browser window or tab. */
  path?: S
  /** When specified, a split button is rendered with extra actions tied to it within a context menu. Mutually exclusive with `link` attribute. */
  commands?: Command[]
}

Example:

Screen.Recording.2023-04-18.at.11.01.48.mov

Closes #1887

@marek-mihok marek-mihok changed the title Feat/issue 1887 feat: Dropdown button #1887 Mar 27, 2023
@marek-mihok marek-mihok marked this pull request as ready for review March 28, 2023 08:56
@marek-mihok marek-mihok requested review from lo5 and mturoci as code owners March 28, 2023 08:56
Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @marek-mihok! Instead of introducing a new ButtonChoice, let's stick with what we already have for these types of scenarios: ui.command.

@marek-mihok
Copy link
Contributor Author

marek-mihok commented Mar 28, 2023

@mturoci originally I've considered it (please, check this commit out) but our existing ui.command does not contain the path property and contains a deprecated data prop. Are you sure with that?

And what about the choices prop? Should I rename it to commands or can I stick with choices?

[EDIT] And ui.command also contains items?: Command[] prop which is not what we want here.

@mturoci
Copy link
Collaborator

mturoci commented Mar 28, 2023

ui.command does not contain the path property and contains a deprecated data prop.

That's not a problem. The point is to have a consistent API since the split button menu is just a collection of commands same as ui.menu or card menu. Is the path prop explicitly required? If yes, we should make it consistent with the rest of the commands as well.

And what about the choices prop? Should I rename it to commands or can I stick with choices?

Go with commands.

And ui.command also contains items?: Command[] prop which is not what we want here.

Why?

@marek-mihok marek-mihok requested a review from mturoci April 11, 2023 21:02
Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @marek-mihok. The spacing looks a bit odd compared to the rest of Wave contextual menus.

image

image

vs

image

@marek-mihok marek-mihok requested a review from mturoci April 18, 2023 09:03
mturoci
mturoci previously approved these changes Apr 18, 2023
Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just need to remove 1 line I missed before and we are good to go.

Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mturoci mturoci merged commit 7388f4b into master Apr 19, 2023
@mturoci mturoci deleted the feat/issue-1887 branch April 19, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dropdown button

2 participants