Skip to content

Conversation

@evgeniyefimov
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Transfrom Alert component to standalone simillar to #8037

@HyperLife1119 HyperLife1119 requested review from HyperLife1119 and removed request for vthinkxie November 22, 2023 18:35
@codecov
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (43b34ae) 91.70% compared to head (abd7012) 91.71%.
Report is 75 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8182   +/-   ##
=======================================
  Coverage   91.70%   91.71%           
=======================================
  Files         519      520    +1     
  Lines       17885    17905   +20     
  Branches     2747     2839   +92     
=======================================
+ Hits        16401    16421   +20     
+ Misses       1182     1181    -1     
- Partials      302      303    +1     

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

Copy link
Collaborator

@HyperLife1119 HyperLife1119 left a comment

Choose a reason for hiding this comment

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

LGTM

@OriginRing
Copy link
Collaborator

Should the use of standalone be revealed in ''When To Use''? @HyperLife1119 @evgeniyefimov

@HyperLife1119
Copy link
Collaborator

Adding two import methods at the same time may cause confusion for users. In order to resolve this confusion, we may also need to use a more detailed explanation, which will be more lengthy. I think just prompting the user that the component supports standalone usage would be enough, WDYT?

@OriginRing
Copy link
Collaborator

OriginRing commented Nov 28, 2023

Adding two import methods at the same time may cause confusion for users. In order to resolve this confusion, we may also need to use a more detailed explanation, which will be more lengthy. I think just prompting the user that the component supports standalone usage would be enough, WDYT?

截屏2023-11-28 16 17 33

I added it in the hashcode before, I think it's okay, and 17 will default to standalone, I think we should also use standalone as the main one

@HyperLife1119
Copy link
Collaborator

Looks good! Maybe we don't need the "Import Module" header anymore.

@evgeniyefimov
Copy link
Contributor Author

I think two types of imports in the docs may really confuse users. It'll be difficult to explain that users need to import multiple components for complex components to let them work correctly instead of a single module. Single module just more user friendly. I think it's better to mention only modules in the docs. It's simillar to Angular Material. Material's components and directives are standalone, but only module import is mentioned in the docs.

I would say that standalone components and directives have next purposes in the lib:

  • simplify internal import dependencies managment
  • provide alternative imports for advanced users (simillar to CommonModule and standalone directives from @angular/common)

@OriginRing
Copy link
Collaborator

I think two types of imports in the docs may really confuse users. It'll be difficult to explain that users need to import multiple components for complex components to let them work correctly instead of a single module. Single module just more user friendly. I think it's better to mention only modules in the docs. It's simillar to Angular Material. Material's components and directives are standalone, but only module import is mentioned in the docs.

I would say that standalone components and directives have next purposes in the lib:

  • simplify internal import dependencies managment
  • provide alternative imports for advanced users (simillar to CommonModule and standalone directives from @angular/common)

For complex components, it is not recommended to support standalone and should be imported as module. @evgeniyefimov

@MunMunMiao
Copy link
Collaborator

I believe that since the official direction is towards standalone components. So we can pursue the official position, which reduces everyone's communication costs and Zorro's future path.

I believe that shifting towards a comprehensive standalone and abandoning the Module is the direction for Angular's future.

Why not focus on future development now that there is community assistance available?

@HyperLife1119
Copy link
Collaborator

HyperLife1119 commented Nov 29, 2023

It's worth noting that while Standalone is taking the lead, NgModule will not be deprecated, and we can do both, leaving it up to the user to decide whether they want to use NgModule or Standalone Component.

Personally, when a feature has a lot of components/directives, I prefer to use NgModule because it's more cohesive and allows me to import all the components I need at once.

Of course, this can be a bit subjective, so the choice should be left to the user, all we need to do is to remind them that our components support standalone mode :)

@ParsaArvanehPA
Copy link
Contributor

The current documentation of the components indicates that they can also be standalone at the API section of each component. However, since standalone approach will be the default approach from Angular version 17, I think it would be better to change the order of the information.
I suggest showing how to import the components as standalone at the beginning of each document, and then stating that they also support module at the API section.
This way, we can align the documentation with the future direction of Angular development.

@OriginRing
Copy link
Collaborator

The current standalone transformation has already started, so is it okay if we support standalone for all components in 17.1.0 and change the documentation and demos to standalone mode in one of the subsequent releases? @HyperLife1119 @MunMunMiao @simplejason

@MunMunMiao
Copy link
Collaborator

The current standalone transformation has already started, so is it okay if we support standalone for all components in 17.1.0 and change the documentation and demos to standalone mode in one of the subsequent releases? @HyperLife1119 @MunMunMiao @simplejason

Yes. Convert all components to standalone, with standalone as the main focus while retaining modules, ensuring that the component library remains consistent with the official development roadmap.
Removing modules will also become easier in the future. So it's not necessary to keep some modules and some standalone.

exportAs: 'nzAlert',
animations: [slideAlertMotion],
standalone: true,
imports: [BidiModule, NgIf, NzIconModule, NzOutletModule],
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: You can remove BidModule because the directive exported by this module is not used here

@Nicoss54 Nicoss54 self-requested a review December 5, 2023 10:06
@Nicoss54
Copy link
Collaborator

Nicoss54 commented Dec 5, 2023

LGTM

@Nicoss54
Copy link
Collaborator

Nicoss54 commented Dec 5, 2023

LGTM

@Nicoss54 Nicoss54 merged commit 167bed0 into NG-ZORRO:master Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants