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.

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

Dynamic Dialog: Fixed errors when is opened #13502

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

SoyDiego
Copy link
Contributor

@SoyDiego SoyDiego commented Aug 15, 2023

Fix #13497

Changed the changeDetection to onPush solves the problem. I don't know if it's the best solution. If you have another, let me know.

Note: Ignore the warning message responsive property is deprecated as table is always responsive with scrollable behavior.

CURRENT BEHAVIOUR

problem dynamic dialog

AFTER SOLUTION

fixed dynamic dialog

@vercel
Copy link

vercel bot commented Aug 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
primeng ⬜️ Ignored (Inspect) Visit Preview Aug 15, 2023 4:49pm

@gucal gucal merged commit 6ce4144 into primefaces:master Aug 18, 2023
2 of 4 checks passed
@MarcinBorowski95
Copy link

I'm not sure if changing the change detection is the best solution here. It could bring some unforeseen consequences.
Wouldn't it be better to:
Instead of using a method like this
[attr.aria-labelledby]="getAriaLabelledBy()"
create a property that will be initialized in a constructor/onInit?
That way change detection will not cause a refresh of labelledBy and then an error.

@SoyDiego
Copy link
Contributor Author

SoyDiego commented Aug 23, 2023

I'm not sure if changing the change detection is the best solution here. It could bring some unforeseen consequences. Wouldn't it be better to: Instead of using a method like this [attr.aria-labelledby]="getAriaLabelledBy()" create a property that will be initialized in a constructor/onInit? That way change detection will not cause a refresh of labelledBy and then an error.

Thanks for check @MarcinBorowski95. In the issue post I said I don't know if it the best solution:
image

I will try your solution and generate a new PR if continue working good.
Thanks for review and comment a possible solution here!
This afternoon I will try to do it! :)

@SoyDiego
Copy link
Contributor Author

SoyDiego commented Aug 23, 2023

Hi @MarcinBorowski95 I'm doing your way:

image

image

And it's true, works and no generate the error but now I have a doubt about this:

image

With your way we have same values on init and in the way of PrimeNG were different.

I don't know if it's correct or not but the error don't show like you said.

If I understood wrong, let me know and thanks again

@MarcinBorowski95
Copy link

I think this is still better. And if different id's are necessary then I guess two values can be generated. It will still be more optimized and less prone to bad change detection issues.

@SoyDiego
Copy link
Contributor Author

I think this is still better. And if different id's are necessary then I guess two values can be generated. It will still be more optimized and less prone to bad change detection issues.

Thanks again bro. I added '_title' to the second one, just in case need to be different

image.

I will do the PR doing a mention to you :)

@MarcinBorowski95
Copy link

Happy to help 😁

@SoyDiego
Copy link
Contributor Author

Happy to help 😁

Here you have the PR, I cannot mentioned directly but I wrote your profile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DynamicDialog: ExpressionChangedAfterItHasBeenCheckedError for attr.aria-labelledby
3 participants