Skip to content
This repository was archived by the owner on Dec 15, 2023. It is now read-only.

feat: dp-46328 callbar buttons Vue3 #471

Merged
merged 6 commits into from
Sep 23, 2022

Conversation

matiasg-dialpad
Copy link
Contributor

PR Title

VUE3 PR for this Vue2 PR: #436

🛠️ Type Of Change

  • Fix
  • Feature
  • Refactoring
  • Documentation

📖 Description

💡 Context

📝 Checklist

  • I have reviewed my changes
  • I have added tests
  • I have added all relevant documentation
  • I have validated components with a screen reader
  • I have validated components keyboard navigation
  • I have considered the performance impact of my change
  • I have checked that my change did not significantly increase bundle size
  • I am exporting any new components or constants in the index.js in the component directory
  • I am exporting any new components or constants in the index.js in the root

🔮 Next Steps

📷 Screenshots / GIFs

🔗 Sources

@matiasg-dialpad matiasg-dialpad force-pushed the mfgea/DP-46328-callbar-button-vue3 branch from 4db1320 to d142bd1 Compare August 29, 2022 21:30
@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

@matiasg-dialpad matiasg-dialpad changed the title Mfgea/dp 46328 callbar button vue3 feat: dp-46328 callbar buttons Vue3 Aug 30, 2022
Copy link
Contributor

@josedialpad josedialpad left a comment

Choose a reason for hiding this comment

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

A couple of things:

  1. The rounded variants are missing the d-btn--icon-only class?

2022-08-30 at 12 44 11@2x

  1. The click event isn't logged in the Actions panel.

@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

@matiasg-dialpad matiasg-dialpad force-pushed the mfgea/DP-46328-callbar-button-vue3 branch from 7ad2e97 to ff912bb Compare August 30, 2022 18:35
@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

@matiasg-dialpad matiasg-dialpad force-pushed the mfgea/DP-46328-callbar-button-vue3 branch from ff912bb to 9ae6ea1 Compare August 30, 2022 18:51
@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

@matiasg-dialpad matiasg-dialpad force-pushed the mfgea/DP-46328-callbar-button-vue3 branch from 9ae6ea1 to 3288cb9 Compare August 30, 2022 18:56
@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

Comment on lines +19 to +24
<dt-recipe-callbar-button>
No tooltip
<template #icon>
<icon-dialpad-glyph />
</template>
</dt-recipe-callbar-button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Displays a blank tooltip instead of no tooltip when hovering. Confirmed this doesn't happen on the vue 2 branch.

Screen Shot 2022-08-30 at 4 26 08 PM

@matiasg-dialpad matiasg-dialpad force-pushed the mfgea/DP-46328-callbar-button-vue3 branch from 2d94ea7 to 1316569 Compare September 13, 2022 16:43
@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

@matiasg-dialpad matiasg-dialpad force-pushed the mfgea/DP-46328-callbar-button-vue3 branch from 1316569 to dbfff30 Compare September 13, 2022 16:48
@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

Copy link
Contributor

@josedialpad josedialpad left a comment

Choose a reason for hiding this comment

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

w/ one comment, good work!

Comment on lines +212 to +224
defaultSlotIsEmpty () {
const defaultSlotContents = this.$slots?.default();
const slotItemCount = defaultSlotContents?.length;

const slotIsEmpty = slotItemCount === 1 &&
(defaultSlotContents[0].type.toString() === 'Symbol(Comment)' ||
(
defaultSlotContents[0].type.toString() === 'Symbol(Fragment)' &&
defaultSlotContents[0].children.length === 0)
);

return !slotItemCount || slotIsEmpty;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this rigorous validation. Maybe leaving this as in the vue 2 implementation (validate that something has been passed to the slot) is enough?

 defaultSlotIsEmpty () {
      const defaultSlotContents = this.$slots?.default();
      const slotItemCount = defaultSlotContents?.length;
      
      return !slotItemCount;
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not work as straightforward as in vue2. default() returns a list of nodes, and if, for example, you pass an empty <slot> or a <template v-if="..."> it will still return a component (empty fragment or comment).

I tried several use cases and this is the one that worked for all of them (I don't say I tested all usecases though, maybe I missed something).

Tested the proposed solution, as well as the inherited vue2 implementation, and it does not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was trying to get a simpler solution but it seems that how you did is the simpler one 👍

@braddialpad
Copy link
Contributor

braddialpad commented Sep 13, 2022

The visual tests are showing the tab active line color has changed as a result of this PR. Confirmed in the preview that this does seem to be the case. Must be a style getting overridden somewhere

Screen Shot 2022-09-13 at 11 25 53 AM

Comment on lines 262 to 264
.d-tab--selected::after, .d-tab--selected:hover::after {
--tab--bgc: var(--black-900);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing this is the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh sorry! I missed some vue basics here. Forgot this is not encapsulated. Will add more specificity.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matiasg-dialpad Hm we need to apply the same fix for the vue2 version.
Wondering in which cases we want to use these classes (.tab-group, .tab-content) and the override in the .d-tab--selected as there aren't usages in the stories and in the design there aren't examples using the tab component?

Copy link
Contributor

Choose a reason for hiding this comment

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

Approved since your last change fixed the visual regression, but yeah I'm also curious why we are using tab classes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ibased on this one, check the popover section
Screen Shot 2022-09-19 at 11 19 30

but there has been some changes in the code, let me chef if those classes are still needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just remove those tab color styles. Since any content could go in the slots for this component I don't think it makes sense for this component to dictate tab color.

What I can do though is add a "muted" styling option to the tab component, I think that's what we're looking for here.

@github-actions
Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-471/
🔨 If you experience an SSL issue then wait 2 minutes and try again.

@matiasg-dialpad
Copy link
Contributor Author

@braddialpad @josedialpad ready for another review! (hope this is the last one... 😬)

@braddialpad
Copy link
Contributor

Merging this in because someone needs to use it

@braddialpad braddialpad merged commit 4861677 into staging-vue3 Sep 23, 2022
@braddialpad braddialpad deleted the mfgea/DP-46328-callbar-button-vue3 branch September 23, 2022 22:47
github-actions bot pushed a commit that referenced this pull request Sep 23, 2022
* feat: DP-46328 call bar buttons

New button look and states for the redesigned call bar buttons from the call workflow revamp project

* Added callbar-button-with-popover component. Cleaned up some of the callbar-button.

* Migrated to Vue3 and fixed tests

* Changed event handling in the component to keep up with Vue3 best practices

* Fix tooltip to not show when there's no content. Fix round callbar buttons

* Add more specificity to the css selectors

Co-authored-by: Felipe Gomes <[email protected]>
braddialpad pushed a commit that referenced this pull request Sep 23, 2022
# [3.18.0](v3.17.0...v3.18.0) (2022-09-23)

### Bug Fixes

* dt-631 remove primary-color from dialtone vue ([#518](#518)) ([28d89ec](28d89ec))

### Features

* dp-46328 callbar buttons Vue3 ([#471](#471)) ([4861677](4861677))
braddialpad pushed a commit that referenced this pull request Sep 26, 2022
* feat: DP-46328 call bar buttons

New button look and states for the redesigned call bar buttons from the call workflow revamp project

* Added callbar-button-with-popover component. Cleaned up some of the callbar-button.

* Migrated to Vue3 and fixed tests

* Changed event handling in the component to keep up with Vue3 best practices

* Fix tooltip to not show when there's no content. Fix round callbar buttons

* Add more specificity to the css selectors

Co-authored-by: Felipe Gomes <[email protected]>

Co-authored-by: Matias Gea <[email protected]>
Co-authored-by: Felipe Gomes <[email protected]>
juliodialpad pushed a commit that referenced this pull request Oct 11, 2022
# [3.19.0](v3.18.0...v3.19.0) (2022-10-11)

### Bug Fixes

* dt-631 remove primary-color from dialtone vue ([#520](#520)) ([6f58dcf](6f58dcf))
* update dialtone peer dependency ([c9f53ef](c9f53ef))

### Features

* (card) vue-3 ([#494](#494)) ([#500](#500)) ([82a5d18](82a5d18))
* dp-46328 callbar buttons Vue3 ([#471](#471)) ([#521](#521)) ([39d9f62](39d9f62))
* dp-57068 Adding independent control for showing the arrow ([#523](#523)) ([104bf0f](104bf0f))
* dp-57068 Adding independent control for showing the arrow ([#524](#524)) ([66c082b](66c082b))
* dp-57757 add `buttonClass` prop to Call Bar Button component (Vue3) ([#538](#538)) ([b36e5c6](b36e5c6))
* dp-57757 add `buttonClass` prop to Call Bar Button component (Vue3) ([#538](#538)) ([#542](#542)) ([d512461](d512461))
* dt-recipe-ivr-node vue3 ([#515](#515)) ([#517](#517)) ([cc56623](cc56623))
* migrate to dialtone 7 ([#507](#507)) ([ab00cd2](ab00cd2)), closes [#504](#504) [#511](#511) [#509](#509)
* migration to dialtone 7 - vue3 ([#440](#440)) ([acdf7e9](acdf7e9))
* **popover:** add sr-only close button - vue3 ([#540](#540)) ([5177cdd](5177cdd))
* **popover:** add sr-only close button - vue3 ([#540](#540)) ([#544](#544)) ([7519248](7519248))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants