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

chore: migration lint #1545

Merged
merged 11 commits into from
Oct 22, 2023

Conversation

davidnixon
Copy link
Collaborator

@davidnixon davidnixon commented Oct 18, 2023

What did you do?

  • enable very strict eslint linting:
     "extends": [
       "eslint:recommended",
       "plugin:vue/vue3-recommended",
       "plugin:prettier-vue/recommended"
     ],
  • automatically fix eslint issue with eslint --fix src/**/*.{js,vue}
  • fix remaining lint issues. Mostly this is about assigning default values to properties:
     sampleString: { type: String, default: undefined },
     sampleArray: { type: Array, default: () => [] }, // mostly but sometimes undefined
     sampleObject: { type: Object, default: undefined } // mostly but sometimes Object()
  • suppress warning in test like [Vue warn]: injection "selectable" not found.
  • When using Vue compatibility mode to migrate a Vue 2 app to Vue 3 with Vue 3 Carbon Components. Some warnings are shown in the console like [Vue warn]: (deprecation ATTR_FALSE_VALUE) Attribute "aria-invalid" .... To resolve many templates are updated like this:
    -        :data-invalid="data.isInvalid"
    +        :data-invalid="data.isInvalid || null"
    See BREAKING: No longer removes attribute if ...

Why did you do it?

  • very strict linting had been on hold until after component migration was complete.
  • working on migration Vue 2 apps now through the end of the year so there may be more warning uncovered by the compatibility compiler.

How have you tested it?

With local app and the jest tests

Were docs updated if needed?

  • N/A

@davidnixon davidnixon marked this pull request as ready for review October 19, 2023 01:01
@davidnixon
Copy link
Collaborator Author

@OlkaB could you review this one? It's a lot of changes but most are related to the strict linting.
Could you run this branch locally and just make sure it looks OK?

@@ -30,8 +30,7 @@ const { disabled, icon, kind, size } = commonCvButtonProps;

export default {
name: 'CvIconButton',
components: { CvSvg },
emits: ['click'], // emitted to allow testing of click
components: { CvSvg }, // emitted to allow testing of click
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment probably should go with moved 'emits' (to line 73)

@@ -44,7 +44,7 @@
<span data-items-selected>
<slot
name="items-selected"
v-bind:scope="{ count: dataRowsSelected.length }"
:scope="{ count: dataRowsSelected.length }"
>{{ dataRowsSelected.length }} items selected</slot
Copy link
Contributor

Choose a reason for hiding this comment

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

side note/question: shouldn't user facing strings be passed as some props or be slotted (for potential language localization purposes)?

:placeholder="placeholder"
:aria-labelledby="uid"
:disabled="disabled ? 'true' : undefined"
Copy link
Contributor

Choose a reason for hiding this comment

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

following compatibility mode comment, might be that also here it should be changed to :disabled="disabled || null" ?

:id="cvId"
:aria-invalid="isInvalid || null"
:aria-describedby="isInvalid ? errorId : undefined"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't there be null instead of undefined?

@@ -30,8 +30,7 @@ const { disabled, icon, kind, size } = commonCvButtonProps;

Copy link
Contributor

@OlkaB OlkaB Oct 20, 2023

Choose a reason for hiding this comment

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

I'm getting this warning CvIconButton story Field. Didn't got that when I run main branch.
image

The value received is an empty string, but alsp seems this prop/attribute is not used in the story at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated story

  1. Get tip alignments fom '../CvTooltip/consts.js';
  2. Fix warnings from story book about deprecated format.
  3. Add control for tipAlignment

@@ -13,22 +13,27 @@ const props = defineProps({
sm: {
Copy link
Contributor

@OlkaB OlkaB Oct 20, 2023

Choose a reason for hiding this comment

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

side note for this component story: it's not working (CvGrid). Interesingly I could run it on main branch without issues.
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is definitely related to the lint changes. For the lint errors I added default: undefined for the break-point size and this is causing the error. Updated to default: false

@@ -2,12 +2,12 @@
<div :class="`cv-pagination ${carbonPrefix}--pagination`" data-pagination>
Copy link
Contributor

Choose a reason for hiding this comment

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

side note: pagination styling seem to fail
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is unrelated to this PR but I "fixed" it.
@felipebritor looks like you added this to .storybook/styles.scss

  // set space between form items
  .bx--form-item {
    margin-bottom: 2rem;
  }

I'm sure I broke something by removing it so maybe have a look and add it to a story instead of to the style?

@OlkaB
Copy link
Contributor

OlkaB commented Oct 20, 2023

@davidnixon Review finished. I've caught some additional issues but they might be better candidates to separate issue tickets.

@davidnixon davidnixon merged commit c8961f4 into carbon-design-system:main Oct 22, 2023
5 checks passed
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