Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Setting Collapse component display input does not work #6070

Closed
marcinmajkowski opened this issue Apr 24, 2021 · 1 comment
Closed

Setting Collapse component display input does not work #6070

marcinmajkowski opened this issue Apr 24, 2021 · 1 comment
Labels
comp(collapse)

Comments

@marcinmajkowski
Copy link

Bug description

Collapse directive has display input but setting its value to e.g. 'flex' does not change display of collapse host - directive still sets it to block.

Reproduction

https://stackblitz.com/edit/angular-hw6kgm?file=app/basic.html

In reproduction, display is set to 'flex' however directive keeps setting style="display: block;".

Additional information

This seem to be related to using TypeScript set (setters) for triggering side effects (instead of doing it "Angular way" using ngOnChanges) - there seem to be race conditions between multiple intputs (display setter may be called before or after isAnimated input is set by the framework and the order seem to be not guaranteed - it even could change between Ivy and View Engine).
marcinmajkowski added a commit to marcinmajkowski/ngx-bootstrap that referenced this issue Apr 24, 2021
@marcinmajkowski
fix(collapse): respect display input value (valor-software#6070)
afe0e89 
This change removes race conditions caused by TypeScript setters being called by Angular in non-deterministic order. Setters side effects (reading values set in other setters) are moved to ngOnChanges lifecycle hook. The consequence is that those side effects are no longer called when those inputs are set other than in the template. This is a breaking change but methods to call instead of setting those properties are already provided (show/hide) so migration path is straightforward. Setting display to 'none' no longer hides the collapse, setting collapse input to true or calling hide method is the way to go.

BREAKING CHANGE: setting display or collapse property on CollapseDirective no longer expands/collapses the collapse - use show/hide methods instead or set collapse input in template
marcinmajkowski added a commit to marcinmajkowski/ngx-bootstrap that referenced this issue Apr 24, 2021
@marcinmajkowski
fix(collapse): respect display input value (valor-software#6070)
6a4ada2 
This change removes race conditions caused by TypeScript setters being called by Angular in non-deterministic order. Setters side effects (reading values set in other setters) are moved to ngOnChanges lifecycle hook. The consequence is that those side effects are no longer called when those inputs are set other than in the template. This is a breaking change but methods to call instead of setting those properties are already provided (show/hide) so migration path is straightforward. Setting display to 'none' no longer hides the collapse, setting collapse input to true or calling hide method is the way to go.

BREAKING CHANGE: setting display or collapse property on CollapseDirective no longer expands/collapses the collapse - use show/hide methods instead or set collapse input in template
@SvetlanaMuravlova SvetlanaMuravlova mentioned this issue Aug 5, 2021
Merged
@marcinmajkowski
Copy link
Author

@SvetlanaMuravlova, I am not sure whether #6257 fixes the issue reliably. There is still the same problem in the code - display setter may run before or after isAnimated input is set, during single change detection cycle. Are you sure this order is somehow guaranteed in Angular? Please look into #6071.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
comp(collapse)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@marcinmajkowski @why-asia @SvetlanaMuravlova