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

v3: experimenting with CvTag component, including stories #1119

Closed
wants to merge 7 commits into from
Closed

v3: experimenting with CvTag component, including stories #1119

wants to merge 7 commits into from

Conversation

Fontinalis
Copy link
Contributor

@Fontinalis Fontinalis commented Feb 13, 2021

Hey @lee-chase,

Seen your commits and decided to join you in the effort of moving forward with the support for Vue 3.

In this PR, I've been experimenting on adding the CvTag component to the v3 package, including stories for documentation. Just something small to begin with.

I see a huge potential in refactoring these components and making them better on the way, extending their features with ones that are already being used in the original React package, but missing from the current @carbon/vue package.

Let me know if you need me to change something, I'm happy to do so.

Changelog

M       packages/v3/package.json
A       packages/v3/src/components/CvTag/CvTag.stories.js
A       packages/v3/src/components/CvTag/CvTag.vue
A       packages/v3/src/components/CvTag/__tests__/CvTag.spec.js
A       packages/v3/src/components/CvTag/consts.js
A       packages/v3/src/components/CvTag/index.js

@netlify
Copy link

netlify bot commented Feb 13, 2021

Deploy preview for carbon-components-vue ready!

Built with commit 5a61f94

https://deploy-preview-1119--carbon-components-vue.netlify.app

@lee-chase
Copy link
Member

This is great @Fontinalis

I wasn't expecting submissions just yet, but more than happy to receive them. Currently I am trying to establish a base expectation for each component. This is likely to include tests that check for non-deprecated features available in V2 in the V3 component.

After which I will go through #1099 and produce an updated contributions guide.

I'll try to get around to your PR next week.

  • It looks like you might have hit prettier, which a yarn format will fix. I use a prettier editor plugin which helps.
  • If you merge in master and run the v3 test you'll see I have added a code coverage monitor. If you can please add tests to your PR as I will be making updates to establish a minimum coverage next week.

Thanks again, and look forward to updates.

@Fontinalis
Copy link
Contributor Author

Absolutely, testing non-deprecated features is essential. And on the same topic, I think there's no better time for dropping support for deprecated props.

Sounds great, let me know if I can help by any means.

  • Yes, yarn format did help.
  • I did the merge and added some tests to the component to achieve the full coverage. BUT, I also did one necessary and an other beauty change in the Jest config.
    1. Added the following part to be able to import and work with @carbon/vue-icons
    "transformIgnorePatterns": [
       "/node_modules/(?!@carbon)"
    ]
    1. Added "!src/components/**/*.stories.js" to the collectCoverageFrom parameter, so the Storybook stories are not in the coverage tests. This way, 100% coverage can be achieved on components with full coverage on the necessary files.

Copy link
Member

@lee-chase lee-chase 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 minor changes and you are good to go.

// - verify tag disabled class on span
const tagSpan = wrapper.find('span');
expect(tagSpan.classes()).toContain(`${carbonPrefix}--tag--disabled`);
});
Copy link
Member

Choose a reason for hiding this comment

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

A direct call to onRemove when the tag is disabled catches the else path.

    // Call onRemove directly to test disabled path
    wrapper.vm.onRemove();

v-if="filter"
:class="`${carbonPrefix}--tag__close-icon`"
:aria-label="clearAriaLabel"
@click="onRemove"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason not to use @click.stop.prevent="onRemove"?

docs: {
source: {
code: `
<CvTag
Copy link
Member

Choose a reason for hiding this comment

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

This is nice will both borrow and raise an issue with Storybook regarding the docs output.

@lee-chase
Copy link
Member

P.S. @Fontinalis found this storybookjs/storybook#13917 and made a suggestion.

@lee-chase lee-chase mentioned this pull request Feb 28, 2021
@Fontinalis Fontinalis deleted the v3/cv-tag branch February 28, 2021 22:03
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.

2 participants