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

fix: add support for formatting vue files #167

Merged
merged 16 commits into from
May 23, 2024

Conversation

felicia-haggqvist
Copy link
Contributor

@felicia-haggqvist felicia-haggqvist commented May 17, 2024

Fixes Jira issue: WARP-520

  • Bump version of @warp-ds/eslint-config devDep
  • Add .eslintrc-file
  • Lint all code using pnpm lint
  • Remove @vue/compiler-sfc devDep because it is now included as a dependency of the main vue package (as of 3.2.13+)
  • Remove eslint-plugin-prettier and eslint-config-prettier as devDependencies since they are already peerDependencies in @warp-ds/eslint-config

@felicia-haggqvist felicia-haggqvist requested a review from a team May 17, 2024 14:08
@felicia-haggqvist felicia-haggqvist self-assigned this May 17, 2024
@felicia-haggqvist felicia-haggqvist changed the title Fix/add support for formatting vue files fix: add support for formatting vue files May 17, 2024
: props.warning
? IconWarning16
: IconInfo16
props.negative ? IconError16 : props.positive ? IconSuccess16 : props.warning ? IconWarning16 : IconInfo16,
Copy link
Collaborator

@magnuh magnuh May 21, 2024

Choose a reason for hiding this comment

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

Can we do something about this? 🙈
(...or maybe this is correct and it would have been better to have it in some other way to start with.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm.... and why did it add a trailing comma? 😅 That's just plain wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree that doesn't look nice! 🙈 I think that is coming from the vue-eslint-parser. Let me see if I can add a rule for that! 🤔

Copy link
Contributor Author

@felicia-haggqvist felicia-haggqvist May 22, 2024

Choose a reason for hiding this comment

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

Ok, so the culprit seems to be the printWidth (once again 🙈 ). If I change the printWidth in the @warp-ds/vue .eslintrc-file to 100 instead of 140 like it is specified in the @warp-ds/eslint-config, it will wrap the lines more frequently. I could add this to the @warp-ds/vue .eslintrc-file:

"rules": {
    "prettier/prettier": [
      "warn",
      {
        "singleQuote": true,
        "trailingComma": "all",
        "printWidth": 100,
        "bracketSameLine": true
      }
    ]
  }

That seems to override the prettier-rules from the @warp-ds/eslint-config. But maybe we don’t want that? It also seemed that I had to add the singeQuote, trailingComma and bracketSameLine, even though I just wanted to override the printWidth from the @warp-ds/eslint-config. For example, I noticed that if I only added the printWidth, then it formatted all of the files in the vue-project to have double-quotes instead of single-quotes like we had specified in @warp-ds/eslint-config.

Another option, that maybe is easier for now, is to add a ”// prettier-ignore” or // eslint-disable-next-line to specific places in this project where we really don’t want to format it to a single line.

And another option is to decrease again the printWidth in the @warp-ds/eslint-config 😄 .

I'm not sure which one we would like to implement? I agree that the readability gets affected here if it moves it all to one single line. @magnuh @imprashast @BalbinaK Wdyt? :)

Copy link
Contributor

@imprashast imprashast May 23, 2024

Choose a reason for hiding this comment

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

I personally hate nested ternary statements like these. Over time they just become impossible to read. I know the alternative to this is a plain old if else block, but atleast that maintains readability:

let icon;
if (props.negative) {
  icon = IconError16;
} else if (props.positive) {
  icon = IconSuccess16;
} else if (props.warning) {
  icon = IconWarning16;
} else {
  icon = IconInfo16;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think changing printWidth to 100 is the solution. We might still see other examples of code that don't wrap though we'd visually expect them to. I'd also consider replacing a nested ternary statement with something a bit more readable at this stage 😄 As for the other rules not applying, not sure what's wrong but we could perhaps look into it together to better understand the problem 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I rambled on a bit too much in my previous comment. 🙈 What I meant before was that when trying to override only the printWidth from the @warp-ds/eslint-config, it overrode all of the other rules too. But I won't add this to the .eslintrc-file, because I don't think we want to override our @warp-ds/eslint-config. :) So, I'll just rewrite this to an if-else instead of a nested ternary statement :)

ccCard.cardOutline,
props.selected ? ccCard.cardOutlineSelected : ccCard.cardOutlineUnselected,
]);
const outlineClasses = computed(() => [ccCard.cardOutline, props.selected ? ccCard.cardOutlineSelected : ccCard.cardOutlineUnselected]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optimally it should leave these kind of things untouched... 🫤 Makes readability a lot worse to have it on one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll also see if I can add a rule in the .eslintrc-file. I think it is the vue-eslint-parser that needs to be tweaked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same issue as this: #167 (comment)

import { computed } from 'vue';

// eslint-disable-next-line vue/no-dupe-keys
import { suffix, prefix } from '@warp-ds/css/component-classes';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should change to import { suffix as ccSuffix, prefix as ccPrefix } from '@warp-ds/css/component-classes'; here, and const classBase = computed(() => (props.prefix ? ccPrefix : ccSuffix)); on line 17?

@@ -16,9 +18,7 @@ const classBase = computed(() => (props.prefix ? prefix : suffix));

const wrapperClass = computed(() => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would have been sooo much nicer. 😄 (But maybe not possible with via the linter 😅 )

const wrapperClass = computed(() => [
  classBase.value.wrapper,
  props.label ? classBase.value.wrapperWithLabel : classBase.value.wrapperWithIcon,
]);

@felicia-haggqvist felicia-haggqvist merged commit 347ab79 into next May 23, 2024
4 checks passed
@felicia-haggqvist felicia-haggqvist deleted the fix/add-support-for-formatting-vue-files branch May 23, 2024 11:42
github-actions bot pushed a commit that referenced this pull request May 23, 2024
## [2.0.1-next.1](v2.0.0...v2.0.1-next.1) (2024-05-23)

### Bug Fixes

* add support for formatting vue files ([#167](#167)) ([347ab79](347ab79))
github-actions bot pushed a commit that referenced this pull request Jul 4, 2024
## [2.0.1](v2.0.0...v2.0.1) (2024-07-04)

### Bug Fixes

* add default targetEl for callout ([#170](#170)) ([957e4cb](957e4cb))
* add support for formatting vue files ([#167](#167)) ([347ab79](347ab79))
* Deprecate the "notification" variant of Badge ([#172](#172)) ([74bf5f2](74bf5f2))
* revert renaming key in w-toggle ([#168](#168)) ([286ed74](286ed74))
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.

4 participants