Skip to content
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

feat: elm draft split button #470

Merged
merged 7 commits into from May 11, 2020
Merged

feat: elm draft split button #470

merged 7 commits into from May 11, 2020

Conversation

juliohr
Copy link
Contributor

@juliohr juliohr commented May 8, 2020

This is the elm implementation of the split button. It is very limited at the moment and does only the necessary for its first usage (survey plans app).

Functionalities and limitations:

  • it has only default variant
  • it expects only an href as the primary action
  • menu items don't support icons, or variant (active, destructive)
  • any click outside of the split button will close the menu
  • escape key press will close the menu

Storybook

@juliohr juliohr requested a review from a team May 8, 2020 01:39
Copy link

@lijuenc lijuenc left a comment

Choose a reason for hiding this comment

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

we have a MenuList component in Elm right?

@sebpearce
Copy link
Contributor

sebpearce commented May 8, 2020

I was thinking the same thing.

This is copied from MenuList kaizen component. This is being used only by the Elm
version since the React version relies on kaizen's MenuList

I'm not sure I understand here. Is there a reason why this component can't also rely on Kaizen's MenuList? There is an Elm one as @lijuenc mentioned. It's at draft-packages/menu-list/KaizenDraft/MenuList/MenuList.elm.

@sebpearce
Copy link
Contributor

sebpearce commented May 8, 2020

Aaaaand I just noticed there's no Storybook story for the Elm MenuList 🤦‍♂️ which could be misleading to someone building a component like this. I'll look into that.

The Elm Select imports the Elm MenuList (via DropdownMenu) so it might be a place to look for an example of that.

keyDecoder =
Decode.map toMsg (Decode.field "key" Decode.string)

toMsg : String -> Msg menuItemMsg
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a key helper in Kaizen.Events that can do this

_ ->
NoOp
in
Sub.batch
Copy link
Contributor

Choose a reason for hiding this comment

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

These subscriptions will be firing for every onClick and keydown on the page even if the split button is not being used. Shouldn't they only fire when a SplitButtonState is open?

@juliohr
Copy link
Contributor Author

juliohr commented May 11, 2020

We wanted to avoid depending on draft things (we wanted to depend only on stable components). If the menuList draft has a major change in the future our darft split button won't block its release. If we need to change the things for this particular menu, we don't need test backwards compatibility with the other areas using the menulist.

Copy link
Contributor

@sebpearce sebpearce left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rasoliveira rasoliveira merged commit 36d1303 into master May 11, 2020
@rasoliveira rasoliveira deleted the jfeijo/elm-split-button branch May 11, 2020 05:30
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.

None yet

5 participants