feat(migrations): add migration to convert ngClass to use class #61971
Conversation
| before: `<div [ngClass]="{'class1': condition1, 'class2': condition2}"></div>`, | ||
| after: `<div [class.class1]="condition1" [class.class2]="condition2"></div>`, |
There was a problem hiding this comment.
Same here, it should be <div [class]="{'class1': condition1, 'class2': condition2}"></div>
| await verifyDeclaration({ | ||
| before: `<div [ngClass]="{'class1 class2': condition}"></div>`, | ||
| after: `<div [class.class1]="condition" [class.class2]="condition"></div>`, |
There was a problem hiding this comment.
I think we should discuss if this migration should be behind a flag.
It can be far from ideal in projects with tailwind when there are several classnames in the key. (could be 10+)
There was a problem hiding this comment.
Ok, how should we discuss it?
There was a problem hiding this comment.
We'll discuss it with the team soon, we'll let you know.
There was a problem hiding this comment.
Ideally this shouldn't be migrated by default.
Maybe we can do that in a follow-up P and behind a prompt with "no" by default.
| await verifyDeclaration({ | ||
| before: `<div [ngClass]="{'admin-panel': isAdmin, 'user-dense': isDense}"></div>`, | ||
| after: `<div [class.admin-panel]="isAdmin" [class.user-dense]="isDense"></div>`, |
There was a problem hiding this comment.
same should be <div [class]="{'admin-panel': isAdmin, 'user-dense': isDense}"></div>
|
@JeanMeche Thanks for the feedback. I'll resolve them. |
| function tryParseStaticObjectLiteral(expr: string): {key: string; value: string}[] | null { | ||
| // Basic object literal regex: matches `{ key: value, ... }` | ||
| const objectLiteralRegex = /^\s*\{\s*([^}]*)\s*\}\s*$/; | ||
| const match = expr.match(objectLiteralRegex); | ||
| if (!match) return null; |
There was a problem hiding this comment.
This should be done using the AST, not a regex.
| it('should migrate single condition', async () => { | ||
| await verifyDeclaration({ | ||
| before: `<div [ngClass]="{'active': isActive}"></div>`, | ||
| after: `<div [class.active]="isActive"></div>`, |
There was a problem hiding this comment.
As highlighted by #45734, this is indeed the right change if there is a single key.
| it('should not migrate object with spread syntax', async () => { | ||
| await verifyDeclarationNoChange(`<div [ngClass]="{foo: true, ...other}"></div>`); | ||
| }); |
There was a problem hiding this comment.
Fwiw, this is not a valid angular syntax. Template don't support the spread operator.
| await verifyDeclarationNoChange(`<div [ngClass]="{foo isActive}"></div>`); | ||
| }); | ||
|
|
||
| it('should not migrate string literal class list', async () => { |
There was a problem hiding this comment.
This should be migrated. [class]="'class1 class2'" is valid.
Ideally this should even be class="class1 class2" Either we need to make sure that we merge with existing class decleration.
| await verifyDeclarationNoChange(`<div [ngClass]="{foo: true, ...other}"></div>`); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Can we also have unit tests that cover the removal of unused NgClass imports
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Added Migration Feature to migrate ngClass to class
What is the current behavior?
Added Migration Feature to migrate ngClass to class
Issue Number:
#61661
What is the new behavior?
It'll migrate ngClass to class bindings to follow the style guide Prefer class over ngClass
Now
<div [class.admin]="isAdmin" [class.dense]="density === 'high'">Before
<div [ngClass]="{admin: isAdmin, dense: density === 'high'}">Does this PR introduce a breaking change?