bug(trace-viewer): Overflow with TabbedPane Component #36251
Conversation
This comment has been minimized.
This comment has been minimized.
|
Hey @agg23 , Did you get a chance to review this PR? |
|
Hey @agg23 , Requesting for a review. |
There was a problem hiding this comment.
Thank you for looking into this (and particularly for creating a test). I am worried that there's a lot of complicated UI code here; we would generally prefer this to be as minimal as possible.
Additionally, it would be nice if the test validated the resize mechanics (the select changing based on available space).
| @@ -77,6 +76,7 @@ | |||
|
|
|||
| .tab-network .tabbed-pane-tab.selected { | |||
| font-weight: bold; | |||
| background-color: var(--vscode-list-inactiveSelectionBackground) !important; | |||
There was a problem hiding this comment.
Why is !important required? If necessary, the reason should be documented, but I think it's probably not necessary.
There was a problem hiding this comment.
Now, that we started using TabbedPane component. So, style:
playwright/packages/web/src/components/tabbedPane.css
Lines 55 to 57 in 23fa813
networkFilters.css.
Which was a white background selection instead of blue background on selection.
So, to honour the blue background selection, added this important here. This style gets applied only on network tab selection on hover.
Seems necessary. Based on the reasoning, do you think I should add a comment?
| {resourceType} | ||
| </div> | ||
| ))} | ||
| <TabbedPane |
There was a problem hiding this comment.
What about other places that use TabbedPane? Do they upgrade to the new functionality for free?
There was a problem hiding this comment.
Yes. I experimented with another component where this worked like a charm. Reference commit: Zemotacqy@8df5e25
| const [visibleTabs, setVisibleTabs] = React.useState<TabbedPaneTabModel[]>(mode !== 'select' ? tabs : []); | ||
| const [overflowTabs, setOverflowTabs] = React.useState<TabbedPaneTabModel[]>(mode === 'select' ? tabs : []); | ||
| const [tabWidths, setTabWidths] = React.useState<Record<string, number>>({}); | ||
| const [, setContainerWidth] = React.useState<number>(0); |
There was a problem hiding this comment.
Are you using this for rerender management? Why does it matter, given you're also setting other state every time this is set? Those other sets will always be new/unique as well, so they will also cause a rerender.
There was a problem hiding this comment.
Thanks @agg23 , This makes sense. I verified, containerWidth wasn't to re-render other states. It was only used for calculation of available space. Have pushed a commit to remove it usage.
| onSelect={setSelectedTab} | ||
| />)), | ||
| ]} | ||
| {visibleTabs.length > 0 && <div style={{ flex: 'auto', display: 'flex', height: '100%', overflow: 'hidden' }} role='tablist'> |
There was a problem hiding this comment.
Does mode continue to work in the same way?
There was a problem hiding this comment.
Yes. mode continues to work in the same way because by default every tab is set as visible tab here:
playwright/packages/web/src/components/tabbedPane.tsx
Lines 50 to 51 in 23fa813
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"6 flaky47041 passed, 979 skipped Merge workflow run. |
|
This is a lot more code that we are comfortable with for such a small feature improvement. Due to the size of our team, we need to optimize heavily for efficiency. Unfortunately that means we are closing this issue for triage. |
Closes: #33930
overflowModeparam that automatically renders elements to a dropdown upon container resize.Before:
before.mp4
After:
after.mp4