Improved UI/UX of find bar #3315
Conversation
|
@jkup Getting close. Here is a spec vs code snap shot and a short list of changes that should get us there:
for colored tab theming:
|
|
let me know when I can take another look @jkup |
|
Cool I'm just gonna get these last design tweaks and then I'll ping you! |
|
This is form my original feedback but the text is glued to the focus ring, could we add some padding to avoid that? I see some black text touching the blue.
|
|
Actually, the blue highlight is in the wrong place. It should be changing the outer box border style, not the inside text field. |
jkup
force-pushed
the
improve-findbar-ui-ux
branch
from
dfaa5cb
to
e7add80
Compare
|
@bbondy I had fixed that but there was a typo in my CSS. The padding should be there now! |
|
It looks like the count not updating isn't fixed yet either. Steps to reproduce was in my previous feedback. (Open search, count updates, close, open again search, count doesn't update) |
|
I think it would be nice to also use getTextColorForBackground for the previous/next button. On some themes I don't think it looks right (example brianbondy.com) |
|
we can do that instead of what I proposed. But the outlines are only for mouse over. But in general, everything can use the GetTextColor for the theme-color tab style. |
|
the right x button as well, please use getTextColorForBackground |
|
On it! |
jkup
force-pushed
the
improve-findbar-ui-ux
branch
from
e7add80
to
7bc200c
Compare
| @@ -60,6 +63,18 @@ class FindBar extends ImmutableComponent { | |||
| input.select() | |||
| } | |||
|
|
|||
| componentWillMount () { | |||
| let stylesheet = document.getElementById('findBarStylesheet') | |||
There was a problem hiding this comment.
Honestly I'd probably get around this problem just by changing the hover state, it's probably not worth having this to spec for the amount of time it can take to do it properly.
However if you want to, I think the proper way is to handle mouseEnter and moueLeave and set state in the findDetail for if it is hovered or not. Then render based on that state.
There was a problem hiding this comment.
Please do get rid of this though ;)
jkup
force-pushed
the
improve-findbar-ui-ux
branch
from
7bc200c
to
3bec3be
Compare
| } | ||
| }, | ||
|
|
||
| stopFindInPage (webview) { |
There was a problem hiding this comment.
does anything actually call this?
There was a problem hiding this comment.
It looks like that's called by onFind in frame.js https://github.com/brave/browser-laptop/blob/master/js/components/frame.js#L1024
There was a problem hiding this comment.
wouldn't this be webviewActions.stopFindInPage vs webview.stopFindInPage?
There was a problem hiding this comment.
I didn't refactor that way because webview.stopFindInPage takes a different parameter currently and I wanted to keep it the same as what it was.
There was a problem hiding this comment.
that makes sense, I'm just trying to figure out where this actually gets called and I'm not seeing a call to webviewActions.stopFindInPage
There was a problem hiding this comment.
fyi @bbondy - the problem with this turned out to be the result of calling stopFindInPage on hide. keepSelection doesn't actually keep any state in the webcontents, it just keeps the selection highlighted on the page. If you want to keep the state you can't call stopFindInPage
bbondy
force-pushed
the
improve-findbar-ui-ux
branch
from
fa0fa6c
to
fc7767f
Compare
| onFindHide () { | ||
| const activeFrame = FrameStateUtil.getActiveFrame(this.props.windowState) | ||
| windowActions.setFindbarShown(activeFrame, false) | ||
| webviewActions.stopFindInPage() |
There was a problem hiding this comment.
@bridiver this had to be called here to retain the existing behaviour.
stopFindInPage needs to be called to clear the highlight but to keep the current match in case you command+g.
There was a problem hiding this comment.
the problem with this is that calling stopFindInPage resets the webcontents find state so you can't get back to the same search results
There was a problem hiding this comment.
but it must keep it because it remembers the next position?
There was a problem hiding this comment.
I think btw the webviewAction was used at one point and was committed out.
There was a problem hiding this comment.
it doesn't remember the next position, that was the bug. I started a slack conversation to explain
There was a problem hiding this comment.
We can't remove the call because when we do the yellow highlight stays forever
bbondy
force-pushed
the
improve-findbar-ui-ux
branch
2 times, most recently
from
bc20777
to
bb4ac34
Compare
Auditors: @jkup The trick to this was to keep track in our app state when electron has find state defined. When it is not defined always do the findFirst call
|
@alexwykoff I'm going to merge this after all since it's ready. |
git rebase -ito squash commits if needed.Fix #3159
Sorry, I got my git history in a really bad state. This is the same UI/UX PR as #3255 but I fixed the bugs outlined by @bbondy.
The last thing I know to need work is the design of the buttons when tab color is in effect. Screenshots posted below. @bradleyrichter can you give me a little feedback on the buttons?
