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(split-button): DLT-1819 vue component #420

Merged
merged 19 commits into from
Aug 7, 2024

Conversation

juliodialpad
Copy link
Collaborator

@juliodialpad juliodialpad commented Jul 30, 2024

Split button - Vue 2 component

This is going to be merged into @francisrupert PR's: #373 which already included most of the CSS and some docs.

Obligatory GIF (super important!)

Obligatory GIF

🛠️ Type Of Change

These types will increment the version number on release:

  • Feature

📖 Jira Ticket

https://dialpad.atlassian.net/browse/DLT-1819

📖 Description

  • Added DtSplitButton Vue 2 component
  • Added Tests
  • Updated yeoman generator templates to match our new standards

📝 Checklist

For all PRs:

  • I have ensured no private Dialpad links or info are in the code or pull request description (Dialtone is a public repo!).
  • I have reviewed my changes.
  • I have added all relevant documentation.
  • I have considered the performance impact of my change.

For all Vue changes:

  • I have added / updated unit tests.
  • I have made my changes in Vue 2 and Vue 3. Note: you may sync your changes from Vue 2 to Vue 3 (or vice versa) using the ./scripts/dialtone-vue-sync.sh script. Read docs here: Dialtone Vue Sync Script
  • I have validated components with a screen reader.
  • I have validated components keyboard navigation.

For all CSS changes:

  • I have used design tokens whenever possible.
  • I have considered how this change will behave on different screen sizes.
  • I have visually validated my change in light and dark mode.
  • I have used gap or flexbox properties for layout instead of margin whenever possible.

If new component:

  • I am exporting any new components or constants:
    • from the index.js in the component directory.
    • from the index.js in the root (either packages/dialtone-vue2 or packages/dialtone-vue3).
  • I have added the styles for the new component to the packages/dialtone-css package.
  • I have created a page for the new component on the documentation site in apps/dialtone-documentation.
  • I have added the new component to common/components_list.cjs
  • I have created a component story in storybook
  • I have created story / stories for any relevant component variants in storybook
  • I have created a docs page for the component in storybook.
  • I have checked that changing all props/slots via the UI in storybook works as expected.

🔮 Next Steps

  • Migrate to Vue 3
  • Add documentation after Vue 3 component is working

📷 Screenshots / GIFs

Screenshot 2024-07-29 at 6 43 10 p m

Copy link

Please add either the visual-test-ready or no-visual-test label to this PR depending on whether you want to run visual tests or not.
It is recommended to run visual tests if your PR changes any UI. ‼️

@juliodialpad juliodialpad added the visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests label Jul 30, 2024
@iropolo
Copy link
Contributor

iropolo commented Jul 30, 2024

Could we rename all alpha to leftBtn and omega to rightBtn? or primary and secondary..
Not sure why the chose of Alpha and Omega, it is not a standard naming, you need some context or think to understand why one button is Alpha and the other Omega, that would be solve with this name update or if there is some other idea..

@iropolo
Copy link
Contributor

iropolo commented Jul 30, 2024

Need to leave now I will continue later, looks good, really cool addition

@iropolo
Copy link
Contributor

iropolo commented Jul 30, 2024

Could be possible to clearly separate Alpha button and Omega button in different components and join it together in the split_button.vue?

The structure would be more readable and they will have more well defined the scope of each, e.g. props.

@juliodialpad
Copy link
Collaborator Author

Could be possible to clearly separate Alpha button and Omega button in different components and join it together in the split_button.vue?

The structure would be more readable and they will have more well defined the scope of each, e.g. props.

It's possible, I'll try and see how it looks.

Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

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

Looks really good! Great variants / examples.

</split-button-omega>
</template>
<template #list>
<!-- @slot Built-in dropdown contents -->
Copy link
Contributor

@braddialpad braddialpad Jul 30, 2024

Choose a reason for hiding this comment

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

bit confused just looking at the doc what should be passed in here. just list items? should it include the <ul> wrapper or not? May also want to mention omega slot will completely override this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the dropdown's list slot, it's not documented properly on storybook either, but I'll make sure to mention this on the dialtone docs.

I'll add a small comment in the storybook slot, but it will be fully documented on dialtone.

@juliodialpad
Copy link
Collaborator Author

Could we rename all alpha to leftBtn and omega to rightBtn? or primary and secondary.. Not sure why the chose of Alpha and Omega, it is not a standard naming, you need some context or think to understand why one button is Alpha and the other Omega, that would be solve with this name update or if there is some other idea..

I was just following the naming convention that @francisrupert already created for the CSS classes in his PR: #373.

I like that naming because if we were using this component on a RTL language, we could potentially invert the order of alpha and beta to make sense in those languages too. I don't know if that was the objective of using that naming, maybe @francisrupert can give us more insight on this.

I can change the naming if it makes this more user-friendly and if @braddialpad and @ninamarina agree with that too.

@braddialpad
Copy link
Contributor

braddialpad commented Aug 1, 2024

I like that naming because if we were using this component on a RTL language, we could potentially invert the order of alpha and beta to make sense in those languages too.

Yes that's correct, we don't want to use right or left, in case we ever support RTL, and when there are multiple elements with no other descriptive distinction referring to them alpha and omega is commonly used. We use this convention in the leftbar component as well. The naming should be fine here.

@francisrupert
Copy link
Contributor

Alpha button should always have a Tooltip, e.g. "More calling options".

@francisrupert
Copy link
Contributor

image

Can the fallback position for omega tooltip be bottom-end?

Copy link
Contributor

@francisrupert francisrupert left a comment

Choose a reason for hiding this comment

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

This is a great implementation. Pretty much all my comments above are individual comments with screenshots, and not necessarily tied to a specific line number.

Let me know if you've any questions.

@juliodialpad
Copy link
Collaborator Author

image Each button size has a specific icon size, can you ensure the proper size is mapped to the button size? I thought this was already built into the default Button component.

image

For Split Button, the omega button icon size was intentionally bumped down:

image

Got it, I was mapping alpha and omega icons the same

@juliodialpad
Copy link
Collaborator Author

At least with the Storybook controls I couldn't figure out how to make the alpha button icon-only.

And related to that, can you add an icon-only group for alpha button on the variants view for each size?

For example...

image

It's a bit strange, but you need to put "" (empty string) instead of just removing the content on default slot so Storybook updated accordingly and you can remove the text from the button. This is just a Storybook thing, this works properly on the component itself.

@juliodialpad
Copy link
Collaborator Author

Alpha button should always have a Tooltip, e.g. "More calling options".

Ok, I'll make alphaTooltipText a required prop.

@julio
Copy link

julio commented Aug 5, 2024 via email

@francisrupert
Copy link
Contributor

Ok, I'll make alphaTooltipText a required prop.

Oop. I meant omega since that's always an icon. For that matter, if alpha is iconOnly, then that requires a Tooltip too.

@francisrupert
Copy link
Contributor

image

When Dropdown is visible, the omega button should be d-btn--active.

We introduced a tiny inconsistency with this update, see screenshot below. Possible solution off the top of my head...

Update this...

// hide divider line when hovering or focusing any button within the split button
.d-split-btn:has(:focus-visible, :hover) & {
opacity: 0;
}

...to...

// hide divider line when hovering or focusing any button within the split button
.d-split-btn:has(:focus-visible, :hover, .d-btn--active) & {
  opacity: 0;
}

...though I'm not sure what that means for the other variants where divider should not be suppressed.

Screen.Recording.2024-08-05.at.4.12.51.PM.mov

@juliodialpad
Copy link
Collaborator Author

any chance you guys can swap me out for the right julio? when you tag @julio, these messages come to me you want to tag @juliodialpad instead thanks

On Mon, Aug 5, 2024 at 1:20 PM Julio Ortega @.> wrote: Alpha button should always have a Tooltip, e.g. "More calling options". Ok, I'll make alphaTooltipText a required prop. — Reply to this email directly, view it on GitHub <#420 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAB7X7WANCLUVVT6GXEJTDZP7NB3AVCNFSM6AAAAABLVJXVOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRZHA2DQMRVGI . You are receiving this because you were mentioned.Message ID: @.>
-- Julio

Sorry for that, you can try unsubscribing, go to the top of the page and click unsubscribe, that'd will do it, I don't think we can do anything to remove you from the notifications list of this PR.

@juliodialpad
Copy link
Collaborator Author

image Can the fallback position for omega tooltip be `bottom-end`?

That was a bug because it didn't had enough space on the bottom either, fixed by adding a decorator on Storybook preview so it has enough space on top and bottom

Copy link

github-actions bot commented Aug 6, 2024

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-420/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-420/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-420/

Copy link
Contributor

@francisrupert francisrupert left a comment

Choose a reason for hiding this comment

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

Sweet! Thanks so much. I'm assuming https://dialtone.dialpad.com/deploy-previews/pr-420/components/split-button.html will be updated once Vue3 equivalent is done so you can doc it.

@juliodialpad
Copy link
Collaborator Author

Sweet! Thanks so much. I'm assuming https://dialtone.dialpad.com/deploy-previews/pr-420/components/split-button.html will be updated once Vue3 equivalent is done so you can doc it.

Correct, I'll update docs with vue 3 version

@juliodialpad juliodialpad merged commit 163bbab into feat/split-button Aug 7, 2024
10 checks passed
@juliodialpad juliodialpad deleted the feat/split-button-vue branch August 7, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants