-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix query change pixels #1065
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 query change pixels #1065
Conversation
|
@malmstein help wanted for the test step that I marked as ❌. I think it'd be no biggie to not have it, but if we did, better |
|
verify URL in search bar is selected - ❌ this check will fail, I didn't manage to make it work This step does not work for me either. In fact, the pixel is not sent after closing / opening the app. I'd argue that it should not be fired since the query hasn't changed. |
malmstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about the steps to test, other than that looks good. Great job @aitorvs !
|
|
||
| override fun onFocusChanged(focused: Boolean, direction: Int, previouslyFocusedRect: Rect?) { | ||
| super.onFocusChanged(focused, direction, previouslyFocusedRect) | ||
| // stop the auto-selection of text after the first focus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't usually leave comments unless they are really necessary. seems like the code is pretty self-explanatory, so I'd say it's safe to remove them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, will do
Did you close the app and re-open or just put it in background and re-open. The former would send the rq.1 pixel just because the app starts with with an empty |
…ents for sending rq.0
a0073db to
efd7e2d
Compare
|
@aitorvs it should not matter if the app was put in background or closed, for pixel purposes it makes no difference and the rq.1 should not be sent. I don't want to delay this much longer so there are two solutions:
|
Done in c935b09 |
malmstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff @aitorvs. Thanks for making the changes!
Task/Issue URL: https://app.asana.com/0/414730916066338/1199568299173487/f
Tech Design URL: https://app.asana.com/0/481882893211075/1199538898514883/f
CC:
Description:
This PR fixes the
rq_xpixels that got broke when removing the double search header. The main issue was that pixels have a suffixandroid_{formFactor}, but also under the iOS implementation a few more fine tuning was done to improve them. This PR will:Steps to test this PR:
Note: to check
rq_xare being sent, just filter forPixel interceptorin logcatThe following should test all scenarios.
rq.xpixels are not sentrq.0pixel is sentrq.0is sentrq.0is sentrq.1pixel is sentrq.xpixels are not sentrq.1pixel is sentrq.1is sentrq.1is sentInternal references:
Software Engineering Expectations
Technical Design Template