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

Fix barcode screen glitch #2702

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Fix barcode screen glitch #2702

wants to merge 3 commits into from

Conversation

avazirna
Copy link
Contributor

@avazirna avazirna commented Sep 1, 2023

Summary

This PR fixes an issue with the Case List screen in which a barcode search view is shown momentarily when the case list is being rendered (see image below).
image

Safety Assurance

  • [ X ] I have confidence that this PR will not introduce a regression for the reasons below

@avazirna avazirna marked this pull request as draft September 1, 2023 21:39
@ctsims
Copy link
Member

ctsims commented Sep 4, 2023

@avazirna I think the code looks good here, but a few points on the PR:

1- This PR is really two meaningful changes (or one change for two big reasons). One is eliminating legacy code that no longer has an execution path, the other is fixing a bug that happens to get fixed when that code is removed.

I'd make sure that both of those intents are communicated here. If someone git blames back to this PR, it would be super confusing for them to see that the reason that 80 lines of code and a bunch of views were removed was to fix a bug where a view temporarily flickered and disappeared.

2 - You have the safety story checked but don't provide an explanation on why it's safe. I think that context does exist from our chats (IE: It's clear looking at Shubham's PR that there used to be a meaningful conditional where a bunch of code wasn't activated, and now there isn't), so it's good to include that background info here so a reviewer can see that this code couldn't possibly be used. Also good to confirm that you've tested the new view layouts on the two screens correctly and confirmed that they behave right still after eliminating the view (and needing to re-anchor on a new view)

@avazirna
Copy link
Contributor Author

avazirna commented Sep 5, 2023

@avazirna I think the code looks good here, but a few points on the PR:

1- This PR is really two meaningful changes (or one change for two big reasons). One is eliminating legacy code that no longer has an execution path, the other is fixing a bug that happens to get fixed when that code is removed.

I'd make sure that both of those intents are communicated here. If someone git blames back to this PR, it would be super confusing for them to see that the reason that 80 lines of code and a bunch of views were removed was to fix a bug where a view temporarily flickered and disappeared.

2 - You have the safety story checked but don't provide an explanation on why it's safe. I think that context does exist from our chats (IE: It's clear looking at Shubham's PR that there used to be a meaningful conditional where a bunch of code wasn't activated, and now there isn't), so it's good to include that background info here so a reviewer can see that this code couldn't possibly be used. Also good to confirm that you've tested the new view layouts on the two screens correctly and confirmed that they behave right still after eliminating the view (and needing to re-anchor on a new view)

@ctsims thanks for the notes, I will update the PR to make sure this removal is well documented. As per the testing, it seems to be working as expected in newer versions of Android, but I still need to test on older versions.

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.

2 participants