feat(filter-pill): DLT-1917 create component #473

juliodialpad · 2024-08-26T20:01:19Z

Add DtFilterPill component

Obligatory GIF (super important!)

🛠️ Type Of Change

These types will increment the version number on release:

Feature

📖 Jira Ticket

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

📖 Description

Added filter-pill component on both Vue 2 and Vue 3 versions
Improved lint-staged config to actually exclude .eslintignore paths.
Added simple filter-pill docs
Added d-filter-pill classes to dialtone-css.

💡 Context

This component is still not ready for production, will not be exported until @francisrupert completes the Figma and Design proposal.

📝 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.

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 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

Comment exports on dialtone-vue
Exclude filter_pill stories from storybook

📷 Screenshots / GIFs

Sources

Lint-staged ignore files: https://github.com/lint-staged/lint-staged?tab=readme-ov-file#eslint--7-1

Links

github-actions · 2024-08-26T21:31:50Z

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. ‼️

francisrupert

Great start! Functionality, props, and slots are as I expected. The design very likely will change the raw html/css.

packages/dialtone-vue2/components/filter_pill/filter_pill.vue

francisrupert

Approved at least in current designed state. Defer to others for technical review.

braddialpad

Looks pretty great so far, will review more tomorrow morning.

braddialpad · 2024-08-27T23:57:10Z

packages/dialtone-vue3/components/filter_pill/filter_pill.vue

+    class="d-filter-pill"
+    data-qa="dt-filter-pill"
+    :open="isPopoverOpen"
+    @update:open="isPopoverOpen = $event"


Shouldn't we just use v-model / sync if we're going to do this?

Also should this component support v-model / sync?

Also should this component support v-model / sync?

Didn't found any use-case on product that had the filter previously opened, or do you mean for something like the "active" state?

packages/dialtone-css/lib/build/less/components/filter-pill.less

braddialpad

Thanks, couple more comments from this morning. I screen reader tested as well, looks good!

braddialpad · 2024-08-28T16:59:19Z

packages/dialtone-vue2/components/filter_pill/filter_pill.vue

+    /**
+     * Determines if the filter is loading
+     */
+    loading: {
+      type: Boolean,
+      default: false,
+    },


Hmm not sure about this loading implementation. Why did we use a skeleton instead of the built in "loading" prop on button that shows a spinner? This looks a bit strange:

There was also no loading prop requirement in the ticket https://dialpad.atlassian.net/browse/DLT-1917. Not to say it won't be useful at some point, but I didn't find any existing implementations of filter pill that show a loading indicator within the button.

Can't find the button I saw using that, but I copied the functionality from there.

Makes sense to use the loading prop of the button as we don't want to keep introducing new loaders.

It made sense for me as there was actually going to be text there, but I'll change it to the loading prop, it's going to be easier to maintain.

Loading prop

Loading skeleton

Actually, the loading prop of the button replaces the entire button content. I'd prefer to maintain the loading skeleton in that case.

@francisrupert can you help us to decide here please?

braddialpad · 2024-08-28T17:00:35Z

packages/dialtone-vue2/components/filter_pill/filter_pill.vue

+    loadingSkeletonWidth: {
+      type: String,
+      default: '128px',
+    },


Changing this changes the entire width of the button rather than the width of the skeleton. Intended?

As the skeleton is growing/shrinking it'd be expected that the button size change, right? The use-case I saw for this was setting the loading-skeleton-width to the actual text-size which could be calculated either on the client or I can add that here but I'd be more flexible if I left the client to decide the width of the loading skeleton.

If the client is receiving the text from a server it won't know its size until the request is complete in which case it is done loading. Calculating width based on the length of a string even if we had it is VERY difficult due to variable width fonts, i18n, and text scaling. Basically the same problem we had in this ticket where we were trying to dynamically change the size of the field based on the language and it was impossible to size it correctly.

https://dialpad.atlassian.net/browse/DP-107950

Ok, setting it to the text size was too ambitious 😅 but having the flexibility to change the size is ok, right?

braddialpad · 2024-08-28T17:06:02Z

packages/dialtone-vue2/components/filter_pill/filter_pill.vue

+<template>
+  <dt-popover
+    class="d-filter-pill"
+    data-qa="dt-filter-pill"


We probably want to pass through "placement" and "fallbackPlacements" prop since they're very commonly used.

placement can default to "bottom-start"
fallbackPlacements can default to ["top-start", "auto"]

A couple other props I think we'll need to pass through

appendTo
maxHeight
maxWidth
padding

Makes sense, I wasn't thinking on those, thanks!

braddialpad · 2024-08-28T17:21:01Z

packages/dialtone-vue2/components/filter_pill/filter_pill.vue

+        <dt-button
+          v-if="showReset"
+          data-qa="dt-filter-pill__reset-button"
+          class="d-filter-pill__reset"
+          circle
+          importance="clear"
+          kind="muted"
+          :size="size"
+          :aria-label="resetButtonAriaLabel"
+          @click="$emit('reset', $event)"
+        >
+          <template #icon="{ iconSize }">
+            <dt-icon-close :size="iconSize" />
+          </template>
+        </dt-button>


Size of the button feels really big and actually overlaps the text at XL size

The button is the same size as the other button, should be ok. What they're doing on product is only changing the "X" icon color but not changing the background color on hover. I can do the same here but I wanted to keep the button implementation.

I thinks this might change with the upcoming @francisrupert updates

Preview of WIP Design, via Draft PR #484.

I think I should wait until you have the final HTML implementation so I can make the appropriate changes here.

Maybe closing this PR and open a new one with the final implementation, what do you think? @francisrupert @braddialpad

sure, or you can just PR a new PR into this branch. I don't think there's really any reason to release this early.

lint-staged.config.cjs

# Conflicts: # packages/dialtone-css/lib/build/less/dialtone.less

github-actions · 2024-09-02T21:59:59Z

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

juliodialpad added 19 commits August 19, 2024 19:04

setup component

94b3ef3

move styles to dialtone-css

6a0e8a4

add loading state

6c60b1c

Merge branch 'staging' into feat/filter-pill-component-vue

e115b65

cleanup

3ee1ecb

add headerContent and footerContent slots

2712981

add examples

b11b1cd

update docs

c435a93

add tests

22c4e87

Vue 3 migration

71ccf1f

cleanup

fb85a47

add docs

a4757e2

add to site-nav

fa2b736

Merge branch 'refs/heads/staging' into feat/filter-pill-component-vue

62d3c73

sync

1725596

test commit

37f3353

update lint-staged config

3135d58

update lint-staged to include .eslintignore

7b07aef

test commit

62b4fce

juliodialpad requested review from iropolo, francisrupert, braddialpad and ninamarina as code owners August 26, 2024 20:01

juliodialpad self-assigned this Aug 26, 2024

add resetButtonAriaLabel

2a738e4

juliodialpad added the no-visual-test Add this tag when the PR does not need visual testing label Aug 26, 2024

francisrupert reviewed Aug 26, 2024

View reviewed changes

packages/dialtone-vue2/components/filter_pill/filter_pill.vue Show resolved Hide resolved

packages/dialtone-vue2/components/filter_pill/filter_pill.vue Show resolved Hide resolved

packages/dialtone-vue2/components/filter_pill/filter_pill.vue Outdated Show resolved Hide resolved

juliodialpad added 2 commits August 27, 2024 10:45

add size prop

45e8d59

add size prop - vue 3

08bc083

juliodialpad added 4 commits August 27, 2024 10:47

update docs

f5ba936

update example

758c78c

add aria-label on loading only

1e43cf6

Merge branch 'staging' into feat/filter-pill-component-vue

196b9a9

francisrupert approved these changes Aug 27, 2024

View reviewed changes

braddialpad reviewed Aug 28, 2024

View reviewed changes

braddialpad requested changes Aug 28, 2024

View reviewed changes

juliodialpad added 4 commits September 2, 2024 12:34

Merge branch 'staging' into feat/filter-pill-component-vue

42c9778

# Conflicts: # packages/dialtone-css/lib/build/less/dialtone.less

update tokens

68534ba

use v-model

e47a20b

add pass-through props

7c01c19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(filter-pill): DLT-1917 create component #473

feat(filter-pill): DLT-1917 create component #473

juliodialpad commented Aug 26, 2024 •

edited

Loading

github-actions bot commented Aug 26, 2024

francisrupert left a comment

francisrupert left a comment

braddialpad left a comment

braddialpad Aug 27, 2024

braddialpad Aug 28, 2024

juliodialpad Sep 2, 2024

braddialpad left a comment •

edited

Loading

braddialpad Aug 28, 2024

juliodialpad Sep 2, 2024

juliodialpad Sep 2, 2024 •

edited

Loading

braddialpad Aug 28, 2024

juliodialpad Sep 2, 2024

braddialpad Sep 5, 2024

juliodialpad Sep 5, 2024

braddialpad Aug 28, 2024

braddialpad Aug 28, 2024

juliodialpad Sep 2, 2024

braddialpad Aug 28, 2024

juliodialpad Sep 2, 2024

francisrupert Sep 3, 2024

juliodialpad Sep 3, 2024

braddialpad Sep 5, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024

feat(filter-pill): DLT-1917 create component #473

Are you sure you want to change the base?

feat(filter-pill): DLT-1917 create component #473

Conversation

juliodialpad commented Aug 26, 2024 • edited Loading

Add DtFilterPill component

Obligatory GIF (super important!)

🛠️ Type Of Change

📖 Jira Ticket

📖 Description

💡 Context

📝 Checklist

🔮 Next Steps

📷 Screenshots / GIFs

Sources

Links

github-actions bot commented Aug 26, 2024

francisrupert left a comment

Choose a reason for hiding this comment

francisrupert left a comment

Choose a reason for hiding this comment

braddialpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braddialpad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliodialpad Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braddialpad Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 2, 2024

juliodialpad commented Aug 26, 2024 •

edited

Loading

braddialpad left a comment •

edited

Loading

juliodialpad Sep 2, 2024 •

edited

Loading

braddialpad Sep 5, 2024 •

edited

Loading