Fixed file browser covering the navigation - #3829
Conversation
|
Hi @dhruvang21, thanks for submitting this PR! I grabbed your changes, and locally, I'm still seeing the issue. Would you be up for taking another look? 
|
|
@brandonpayton I've fixed the issue in the latest commit. fix-3813.mp4 |
| .tabPanelWrapper { | ||
| display: flex; | ||
| flex: 1 1 auto; | ||
| min-height: 0; | ||
| min-width: 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this used anywhere? Can it be removed?
| overflow-y: hidden; | ||
| -webkit-overflow-scrolling: touch; | ||
| position: relative; | ||
| z-index: 2; |
There was a problem hiding this comment.
Do the z-index additions in this PR solve any problem? As far as I can tell, they aren't needed and can be removed.
| -webkit-overflow-scrolling: touch; | ||
| position: relative; | ||
| z-index: 2; | ||
| background: #ffffff; |
There was a problem hiding this comment.
Is this background setting needed?
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
Thanks for following up on this PR, @dhruvang21.
I took a look at the changes and made a lot of notes. A lot appears removable, and I left a suggestion for what may end up being the simplest fix here.
Would you be up for taking another look at this?
| position: relative; | ||
| isolation: isolate; | ||
| flex: 1 1 auto; | ||
| min-height: 0; |
There was a problem hiding this comment.
I tried removing these changes, and it didn't seem to make a difference for the fix. Can this be removed?
| overflow-x: auto; | ||
| overflow-y: hidden; | ||
| -webkit-overflow-scrolling: touch; | ||
| position: relative; |
There was a problem hiding this comment.
Is this solving any problem? I believe it can be removed without compromising the fix.
| overflow-x: auto; | ||
| overflow-y: hidden; |
There was a problem hiding this comment.
@dhruvang21 Based on my understanding and testing, this removal is what makes the tabs render properly again.
I think maybe we want to keep the overflow-x setting in order to allow horizontal scrolling the tabs when the settings pane is narrow enough to hide some of them.
I think it probably makes sense to restore these lines and add a flex-shrink: 0; here so the flexbox layout does not shrink the tabs container. That works to preserve both tab visibility and horizontal scrolling in my testing.
What do you think?
There was a problem hiding this comment.
@brandonpayton yes, your approach is correct. I applied the changes you suggested by restoring the overflow-x styles and adding flex-shrink: 0; to the tabs container. It preserves horizontal scrolling when needed and fixes the tab rendering issue as expected.
I've pushed these changes in my latest commit. Could you please review it when you have a chance?
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
This looks good. Let's ship it! Thanks, @dhruvang21!
Well... some CI tests are failing, but I think that's due to flakey tests. Let's rerun and confirm. |
Motivation for the change, related issues
Issue: #3813 - "File browser tab and settings panels can cover the Settings navigation tabs in the Site Manager"
Implementation details
Updated CSS
Testing Instructions (or ideally a Blueprint)