-
Notifications
You must be signed in to change notification settings - Fork 975
Conversation
Looking good so far, here's some feedback:
|
I'm having a really tough time figuring this one out. The findbar in production is position:absolute so it doesn't cause this problem but essentially what's happening is if you navigate to another tab and come back to one with the findbar it's pushing the urlbar out of the viewport. If you, for example, remove overflow:hidden from #window you'll see it's still there, just being pushed out of view. Anyone have an idea on how to fix? I'll update the PR with the rest of the fixes but this bug is also what's causing the test to fail. |
@bsclifton do you think you could take a look? |
Auditors: @jkup
Fixed in your branch, the main problem is the UI structure changed so it needed to be reflected in the component hierarchy. In particular it should be part of the top bar now but before it was overlaid on top of the frame. Minor tweaks may be needed but I did some testing and the basics work. I think the above points other than the issue with urlbar hiding needs to be still fixed though. |
Nice fix @bbondy 😄 def makes sense in retrospect moving it out of frame and into main. The same find bar at can be used across multiple frames |
…into improve-findbar-ui
…into improve-findbar-ui
git rebase -i
to squash commits if needed.Fix #3159
cc @bradleyrichter @bbondy
Wanted to put up a work in progress of my ticket so far with a few comments/questions I had.
This is how the whole thing looks right now: