Properly handle URL bar icon when searching and focus changes #6319
Conversation
|
@bsclifton Let's check and pull this one. Nice fix...thanks to @gyandeeps ! |
|
awesome, thanks @gyandeeps 😄 I'll make sure we review this for our next release (we're a little backed up on PRs- hang in there with us 😄 ) |
| @@ -11,6 +11,19 @@ const dndData = require('../../../js/dndData') | |||
| const {isSourceAboutUrl} = require('../../../js/lib/appUrlUtil') | |||
| const searchIconSize = 16 | |||
|
|
|||
| const getIconCssClass = (ctx) => { | |||
| if (ctx.isSearch) { | |||
There was a problem hiding this comment.
I'd add !ctx.isAboutPage to stop returining fa-search fa-list.
There was a problem hiding this comment.
I am not able to understand the use case here. Can you please explain the issue u r seeing?
There was a problem hiding this comment.
ah sorry, I have pushed a PR to place the icon at the center of the input bar, and adding !ctx.isAboutPage avoids the CSS hack for fa-search to push 1px downward being applied to the url bar icon fa-list on about:preferences. https://github.com/brave/browser-laptop/pull/6292/files#diff-f5d8e85b88bbb4fdd8ebaf50901ebb3fR69 and https://github.com/brave/browser-laptop/pull/6292/files#diff-02c4b23ad267fe636760179e32fa29ceR830
There was a problem hiding this comment.
still because it's not the issue which this PR tries to resolve, you might skip it. I'll take care of it then
There was a problem hiding this comment.
k. But i dont think you need to make this change since now its if-else so only one class gets returned by this function. So you can never have a fa-search fa-list together.
There was a problem hiding this comment.
I'm not quite sure how it does work but do you mean ctx.isSearch && !ctx.isAboutPage is not necessary? I'm thinking the case where you try to search something on the URL bar on about:preferences.
There was a problem hiding this comment.
yes and when you try to search something on about:preference page then it will hit the search if condition on top and start showing the search icon. because that what you want when you are searching doenst matter on which page u r on. That how i am thinking about this.
There was a problem hiding this comment.
CC @bradleyrichter for this change
There was a problem hiding this comment.
I agree. The search icon should be displayed any time you are searching regardless of previous/current page because the URL bar mode changes.
There was a problem hiding this comment.
@gyandeeps I tested on the latest master and it looks good on about:prefenreces :-D
bsclifton
force-pushed
the
0.13.1-branch
branch
from
4abf18d to
e29888d
Compare
bsclifton
changed the title
There was a problem hiding this comment.
While reviewing the tests, I did notice one of them is failing.
@gyandeeps can you fix this webdriver test?
npm run test -- --grep="navigationBar tests lockIcon Shows insecure URL icon"
If you haven't ran our webdriver tests before, you'd just have two terminal instances; one of them you'd run npm run watch-test and the other you'd run the above 😄
In this case, it's supposed to be showing the insecure icon... but it's showing NO icon (blank white space). NOTE: If you're on Windows, the webdriver tests won't work; let me know in that case
bsclifton
force-pushed
the
0.13.1-branch
branch
2 times, most recently
from
0da267f to
7f84176
Compare
|
Rebasing this one right now and then I'll check out the failing test 😄 |
|
This likely still has a broken test... but couldn't confirm 100% because of a broken chromedriver dependency after doing fresh npm install. I'll investigate / fix tomorrow morning 😄 |
I missed this with #6319 but our tests caught it :) Auditors: @gyandeeps
I missed this with #6319 but our tests caught it :) Auditors: @gyandeeps
I missed this with brave#6319 but our tests caught it :) Auditors: @gyandeeps
I missed this with #6319 but our tests caught it :) Auditors: @gyandeeps
I missed this with #6319 but our tests caught it :) Auditors: @gyandeeps
I missed this with #6319 but our tests caught it :) Auditors: @gyandeeps
I missed this with #6319 but our tests caught it :) Auditors: @gyandeeps
git rebase -ito squash commits (if needed).Fixes #6273
Test Plan: