Skip to content

refactor(module:*): remove ngClass and ngStyle#8895

Merged
Laffery merged 1 commit into
NG-ZORRO:masterfrom
HyperLife1119:refactor/remove-ngclass-ngstyle
Nov 26, 2024
Merged

refactor(module:*): remove ngClass and ngStyle#8895
Laffery merged 1 commit into
NG-ZORRO:masterfrom
HyperLife1119:refactor/remove-ngclass-ngstyle

Conversation

@HyperLife1119
Copy link
Copy Markdown
Collaborator

@HyperLife1119 HyperLife1119 commented Nov 25, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

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
  • Application (the showcase website) / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

angular/angular#40623
angular/angular#58860

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

All nzClass / nzStyle input properties no longer support the following features:

  • Set(): use arrays instead
  • Keys which multiple styles/classes separated with keys: split a key with spaces into multiple keys

Other information

@zorro-bot
Copy link
Copy Markdown

zorro-bot Bot commented Nov 25, 2024

This preview will be available after the AzureCI is passed.

@Laffery Laffery added this to the backlog milestone Nov 25, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.88%. Comparing base (4498af0) to head (07c944a).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8895      +/-   ##
==========================================
- Coverage   91.89%   91.88%   -0.02%     
==========================================
  Files         547      547              
  Lines       19364    19359       -5     
  Branches     2866     2866              
==========================================
- Hits        17795    17788       -7     
- Misses       1254     1255       +1     
- Partials      315      316       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Collaborator

@Laffery Laffery left a comment

Choose a reason for hiding this comment

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

LGTM

It looks like that angular/angular#58860 isn't in a specific angular's version milestone yet, I'll put it in the backlog milestone first, and later we can put it in v19.1

@HyperLife1119
Copy link
Copy Markdown
Collaborator Author

HyperLife1119 commented Nov 25, 2024

In fact, angular/material already did this migration: https://github.com/angular/components/pulls?q=is%3Apr+remove+dependency+on+NgClass

I guess we don't have to wait v19.1 :)

@HyperLife1119 HyperLife1119 force-pushed the refactor/remove-ngclass-ngstyle branch from 4415d93 to d67bdff Compare November 25, 2024 06:21
@JeanMeche
Copy link
Copy Markdown

JeanMeche commented Nov 25, 2024

Hi, I saw that you were already proactive on this.
To give a bit of insight, this is more likely to be a soft-deprecation to hint to users that they can drop the directive based class/style bindings.

The native bindings almost support the same usecases at the exception of 2 :

  • They don't support Set
  • They don't support keys with multiple styles/classes separated with spaces.

But anyway, you can go ahead with it, as it'll pull less code by dropping the imports of the 2 directives :)

@HyperLife1119 HyperLife1119 force-pushed the refactor/remove-ngclass-ngstyle branch from d67bdff to 07c944a Compare November 25, 2024 06:30
@HyperLife1119
Copy link
Copy Markdown
Collaborator Author

Yes, this is in fact a breaking change, thanks for the insight! @JeanMeche 👍

@HyperLife1119 HyperLife1119 added 💔 Breaking Change This PR or the solution to this issue would introduce breaking changes PR: target-major PR: unreviewed and removed PR: target-minor PR: reviewed-approved labels Nov 25, 2024
@JeanMeche
Copy link
Copy Markdown

You can still target a minor with this (or even a patch), as it shouldn't break anything if you replace correctly the usages :)

@HyperLife1119
Copy link
Copy Markdown
Collaborator Author

You can still target a minor with this (or even a patch), as it shouldn't break anything if you replace correctly the usages :)

Since we also allow ngClass/ngStyle values ​​to be passed in by users, not just for internal use, this is a breaking change in our opinion :)

Copy link
Copy Markdown
Collaborator

@Laffery Laffery left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💔 Breaking Change This PR or the solution to this issue would introduce breaking changes PR: reviewed-approved PR: target-major

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants