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(tag): migrate tag component (VIV-944) #1100

Merged
merged 39 commits into from
Apr 13, 2023
Merged

Conversation

rinaok
Copy link
Contributor

@rinaok rinaok commented Apr 3, 2023

No description provided.

@rinaok rinaok self-assigned this Apr 3, 2023
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #1100 (55d0361) into main (d61b119) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              main     #1100     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files          123       234    +111     
  Lines         1562      2571   +1009     
  Branches       108       139     +31     
===========================================
+ Hits          1562      2571   +1009     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
libs/components/src/lib/badge/badge.ts 100.00% <ø> (ø)
libs/components/src/lib/banner/banner.ts 100.00% <ø> (ø)
libs/components/src/lib/breadcrumb/breadcrumb.ts 100.00% <ø> (ø)
...omponents/src/lib/calendar-event/calendar-event.ts 100.00% <ø> (ø)
libs/components/src/lib/calendar/calendar.ts 100.00% <ø> (ø)
libs/components/src/lib/card/card.ts 100.00% <ø> (ø)
libs/components/src/lib/elevation/elevation.ts 100.00% <ø> (ø)
libs/components/src/lib/fab/fab.template.ts 100.00% <ø> (ø)
libs/components/src/lib/focus/focus.ts 100.00% <ø> (ø)
libs/components/src/lib/header/header.template.ts 100.00% <ø> (ø)
... and 71 more

... and 128 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rinaok rinaok requested review from olaf-k and YonatanKra April 9, 2023 14:25
@rinaok rinaok added Focus: Vivid Parity Parity with vivid components version < 3 and removed work-in-progress labels Apr 9, 2023

Set the `appearance` attribute to change the tag's appearance.

- Type: `'subtle'` | `'duotone'`
Copy link
Contributor

Choose a reason for hiding this comment

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

why subtle instead of filled as used for buttons or avatars? (especially considering that duotone tags look more subtle than the filled ones ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@olaf-k filled is different in its color shades.

```html preview
<vwc-tag label="disabled" disabled></vwc-tag>
```

Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason why it couldn't be both? (looks like removable "wins" at this point.)

libs/components/src/lib/tag/tag.ts Outdated Show resolved Hide resolved
YonatanKra
YonatanKra previously approved these changes Apr 13, 2023
Copy link
Contributor

@YonatanKra YonatanKra left a comment

Choose a reason for hiding this comment

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

See one suggestion. Other than that, fine by me :)

libs/components/src/lib/tag/tag.spec.ts Outdated Show resolved Hide resolved
Comment on lines 113 to 119
#select = (): void => {
if (!this.selectable || this.disabled || this.removable) {
return;
}
this.selected = !this.selected;
this.$emit('selected-change');
};
Copy link
Contributor

@YonatanKra YonatanKra Apr 13, 2023

Choose a reason for hiding this comment

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

I like this solution even better than writing tests for it!
image

Co-authored-by: Yonatan Kra <yonatan.kra@vonage.com>
@rinaok rinaok linked an issue Apr 13, 2023 that may be closed by this pull request

Set the `appearance` attribute to change the tag's appearance.

- Type: `'subtle'` | `'duotone'`
Copy link
Contributor

Choose a reason for hiding this comment

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

@olaf-k filled is different in its color shades.

libs/components/src/lib/tag/ui.test.ts Outdated Show resolved Hide resolved
@rinaok
Copy link
Contributor Author

rinaok commented Apr 13, 2023

@AyalaBu Found the original design from vivid-2
Vonage/vivid#1016
Are you sure we don't need sizes?

Copy link
Contributor

@rachelbt rachelbt left a comment

Choose a reason for hiding this comment

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

LGTM - lets see it inside tag-group :)

@rinaok rinaok merged commit ee15a08 into main Apr 13, 2023
@rinaok rinaok deleted the VIV-944-migrate-tag-component branch April 13, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus: Vivid Parity Parity with vivid components version < 3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[chips]: research and possibly recreation
4 participants