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

[NG] fix alert visibility issue #2971

Merged
merged 1 commit into from Jan 3, 2019

Conversation

Projects
None yet
4 participants
@coryrylan
Copy link
Contributor

coryrylan commented Dec 28, 2018

In prod mode the first alert in alert list is not visible to the user. This happens when using enableProdMode() and is unrelated to AOT.

The check isHidden() only happens once during prod mode while in dev happens multiple times. In production it looks like isHidden is called before the QueryList of the parent alerts component has updated. So when isHidden checks to see if there are any alerts in the QueryList is empty the first time. Every subsequent check seems to work normally.

I found a "fix" but I don't think its a good solution. The code change checks if the list is empty then it can assume its the first alert in the alert list and can fall into the correct if block. Any feedback or ideas would be appreciated.

closes #2430

Signed-off-by: Cory Rylan crylan@vmware.com

[NG] fix alert visibility issue
In prod mode first alert in alert list is not visible to the user.

closes #2430

Signed-off-by: Cory Rylan <crylan@vmware.com>

@coryrylan coryrylan self-assigned this Dec 28, 2018

@Shijir

Shijir approved these changes Jan 2, 2019

Copy link
Contributor

Shijir left a comment

I kinda see why this change is solving the issue.

But there are some weird stuff going on in this component. I didn't know we were manually checking property changes and triggering change detection in this component. We are calling change detection while accessing a property. That's very weird. I don't know the reasoning behind that. Probably we were trying to escape the chococalte error 🤔

@Shijir

This comment has been minimized.

Copy link
Contributor

Shijir commented Jan 2, 2019

Actually, I just tracked the component's history and the PR that introduced this component. Eudes actually commented on the very code block here: #1530 (comment)
So you might also want to take a look into using Oompa-Loompa to solve the issue.

@gnomeontherun

This comment has been minimized.

Copy link
Contributor

gnomeontherun commented Jan 2, 2019

This logic is strange, and I think we can do better. If we agree to this fix for now, we should open a new ticket to refactor this logic and get away from running change detection this way. I'd be ok with running that fix through at the moment, if we commit to a follow up in short order.

@jeeyun

jeeyun approved these changes Jan 3, 2019

Copy link
Contributor

jeeyun left a comment

Strange that this lifecycle issue happens only in prod mode. I wonder what makes the difference. Great find on the cause of the issue. I'm okay with the given fix. I think the comment makes it clear enough and the fix is still easy to understand.

@coryrylan coryrylan merged commit 399ea3b into vmware:master Jan 3, 2019

1 of 2 checks passed

deploy/netlify Deploy preview failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@coryrylan coryrylan deleted the coryrylan:topic/alert-fix branch Jan 3, 2019

@gnomeontherun gnomeontherun added this to the 1.0.4 milestone Jan 10, 2019

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