Skip to content

fix(module:calendar): style radio button not apply #8298

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

Conversation

Nicoss54
Copy link
Collaborator

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] 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?

In calendar component, radio have a specific class todisplay the radio input as a button. This class is applied when the NzRadioButtonDirective is injected

Unfortunatly we just import NzRadioButton so the NzRadiButton directive injection is always null and the correct class is not apply

This is due to the standalone migration

At the end NzRadioButtonDirective must have the NzRadioButton as a host directive

Issue Number: N/A

What is the new behavior?

Correct radio button class is apply

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

Copy link

zorro-bot bot commented Dec 14, 2023

This preview will be available after the AzureCI is passed.

@Nicoss54
Copy link
Collaborator Author

Nicoss54 commented Dec 14, 2023

@HyperLife1119 @ParsaArvanehPA this PR is important because it fix a regression on UI due to the standalone migration. We can't release a new version of Ng-Zorro without this PR

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d287c47) 91.73% compared to head (52dde1c) 91.73%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8298   +/-   ##
=======================================
  Coverage   91.73%   91.73%           
=======================================
  Files         520      520           
  Lines       18009    18009           
  Branches     2751     2838   +87     
=======================================
  Hits        16520    16520           
  Misses       1186     1186           
  Partials      303      303           

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

@ParsaArvanehPA
Copy link
Contributor

ParsaArvanehPA commented Dec 15, 2023

In the What is the current behavior section you have said:

At the end NzRadioButtonDirective must have the NzRadioButton as a host directive

If I understood your solution correctly:
It's not possible to set NzRadioComponent as a hostDirective for NzRadioButtonDirective since it is not a directive but a component.

The alternative is converting NzRadioButtonDirective to a component, and calling nz-radio in it's template, this new component should have all the inputs and outputs of NzRadioComponent and needs to pass them to it in it's template.
I find this syntax extremely unsatisfying, is there any other way in your opinion? @Nicoss54 @HyperLife1119

And btw imho, if the underlying problem is fixed, this pr may no longer be required, so maybe just wait for the solution...

@Nicoss54
Copy link
Collaborator Author

In the What is the current behavior section you have said:

At the end NzRadioButtonDirective must have the NzRadioButton as a host directive

If I understood your solution correctly: It's not possible to set NzRadioComponent as a hostDirective for NzRadioButtonDirective since it is not a directive but a component.

The alternative is converting NzRadioButtonDirective to a component, and calling nz-radio in it's template, this new component should have all the inputs and outputs of NzRadioComponent and needs to pass them to it in it's template. I find this syntax extremely unsatisfying, is there any other way in your opinion? @Nicoss54 @HyperLife1119

And btw imho, if the underlying problem is fixed, this pr may no longer be required, so maybe just wait for the solution...

That’s true, the propose solution can’t work. I read too fast and don’t see that radio is a component. Thanks for your review !! :)

So the second solution is not good enough too because it will be quite complicated to maintain for the future.

By the way this PR can’t be closed and should be merge because currently there is a a regression on the calendar component. The radio button style is not applied.

Let merge this PR when it is approved then we will find a solution to refactor correctly the system of NzRadioComponent, because if we release a minor version of the library with all the standalone components we will introduce a regression for sure and this should’nt be happen. Moreover this fix is just importing the NzRadioModule like it was the case when all the library was not in standalone component. So it’s just do a “rollback”

@Nicoss54
Copy link
Collaborator Author

In the What is the current behavior section you have said:

At the end NzRadioButtonDirective must have the NzRadioButton as a host directive

If I understood your solution correctly: It's not possible to set NzRadioComponent as a hostDirective for NzRadioButtonDirective since it is not a directive but a component.
The alternative is converting NzRadioButtonDirective to a component, and calling nz-radio in it's template, this new component should have all the inputs and outputs of NzRadioComponent and needs to pass them to it in it's template. I find this syntax extremely unsatisfying, is there any other way in your opinion? @Nicoss54 @HyperLife1119
And btw imho, if the underlying problem is fixed, this pr may no longer be required, so maybe just wait for the solution...

That’s true, the propose solution can’t work. I read too fast and don’t see that radio is a component. Thanks for your review !! :)

So the second solution is not good enough too because it will be quite complicated to maintain for the future.

By the way this PR can’t be closed and should be merge because currently there is a a regression on the calendar component. The radio button style is not applied.

Let merge this PR when it is approved then we will find a solution to refactor correctly the system of NzRadioComponent, because if we release a minor version of the library with all the standalone components we will introduce a regression for sure and this should’nt be happen. Moreover this fix is just importing the NzRadioModule like it was the case when all the library was not in standalone component. So it’s just do a “rollback”

Maybe a solution can be to remove a NzRadioButton Directive or make it deprecated first then have a new input for the NZRadioComponent called nzType which can be typed as default | button and apply style based on this input. The problem with that is: it will have breaking change for the user and a schematics need to be write.

@HyperLife1119
Copy link
Collaborator

Here is a possible solution: https://stackblitz.com/edit/stackblitz-starters-wb3uaf

It no longer requires an NzRadioButtonDirective, but uses an Input to determine which selector is currently used.

WDYT?

@Nicoss54
Copy link
Collaborator Author

Here is a possible solution: https://stackblitz.com/edit/stackblitz-starters-wb3uaf

It no longer requires an NzRadioButtonDirective, but uses an Input to determine which selector is currently used.

WDYT?

Yeah this can work without any breaking change for the user. I can implement it in other one.

But at this end this PR is still useful because we will remove the NzRadioButton directive so the NzRadioModule will import only NzRadioComponent and NzRadioGroupComponent and expert them.

The calendar component need both so it make more sense to import the NzRadioModule instead importing the two component separately

what do you thing ?

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

@HyperLife1119 HyperLife1119 merged commit 996e141 into NG-ZORRO:master Dec 15, 2023
@Nicoss54 Nicoss54 deleted the fix/calendar-radio-button-style-regression branch December 17, 2023 17:58
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.

3 participants