Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

feat: add no-clr-alert rule to detect the usage of Clarity Angular alerts #5279

Merged
merged 1 commit into from Nov 10, 2020

Conversation

sis0k0
Copy link
Contributor

@sis0k0 sis0k0 commented Nov 9, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • clarity.design website / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The usage of Clarity Angular alerts is not being detected.

What is the new behavior?

The ESLint plugin (@clr/eslint-clarity-migration) now has a rule detecting the usage of Clarity Angular alerts.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

@hippee-lee hippee-lee left a comment

Choose a reason for hiding this comment

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

This lgtm. I had a couple of questions but nothing that seems like it should block.

return {
'HTMLElement[tagName="clr-alert"]'(node: HTMLElement): void {
context.report({
node: node as any,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there issues that prevent this from being typed as HTMLElement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the HTMLElement type is incompatible with the TSESTree type as defined by the @typescript/eslint library.

const classes = classNode?.attributeValue?.value?.split(' ') || [];
if (classes.includes('alert')) {
context.report({
node: node as any,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questionas line #30.

@@ -1,24 +1,14 @@
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
import { JSDOM } from 'jsdom';
import { SourceLocation } from '@typescript-eslint/eslint-plugin';
import { getDecoratorPropertyValue } from '../utils';
import { calculateLocation, getDecoratorTemplate } from '../utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

output?: string;
}

interface Location {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth using this type in the calculateLocation utility function as something named

ElementLocation || TemplateLocation

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking into this for a while, I've found an appropriate type exported by @typescript-eslint and used it instead.

Copy link
Contributor

@bdryanovski bdryanovski left a comment

Choose a reason for hiding this comment

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

LGTM

…erts

Signed-off-by: Stanimira Vlaeva <svlaeva@vmware.com>
@sis0k0 sis0k0 merged commit 59327da into vmware-archive:next Nov 10, 2020
@sis0k0 sis0k0 deleted the eslint-rule/alert branch November 10, 2020 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants