Copilot commented on code in PR #64013:
URL: https://github.com/apache/airflow/pull/64013#discussion_r3025348137
##########
airflow/www/static/js/dag/details/index.tsx:
##########
@@ -425,8 +429,8 @@ const Details = ({
</Button>
)}
</TabList>
- <TabPanels height="100%">
- <TabPanel height="100%">
+ <TabPanels flex={1} overflowY="auto">
Review Comment:
Right now there are multiple nested scroll containers: `TabPanels` has
`overflowY="auto"`, and several tab contents (e.g. `EventLog`, `DagContent`,
`LogBlock`, `XcomCollection`) also implement their own `maxHeight: calc(100% -
offsetTop)` + `overflowY="auto"`. This can lead to double scrollbars and
awkward UX. Consider making `TabPanels` non-scrollable and letting each tab
manage its own scroll (consistent with the existing `useOffsetTop` pattern), or
conversely remove inner scroll handling for tabs that can rely on `TabPanels`
as the single scroll container.
```suggestion
<TabPanels flex={1}>
```
##########
airflow/www/static/js/dag/details/index.tsx:
##########
@@ -425,8 +429,8 @@ const Details = ({
</Button>
)}
</TabList>
- <TabPanels height="100%">
- <TabPanel height="100%">
+ <TabPanels flex={1} overflowY="auto">
Review Comment:
`TabPanels` is now the scroll container (`overflowY="auto"`), but it’s
missing `minH={0}`. In a column flex layout this can prevent the panels area
from shrinking and reintroduce the overflow behavior you’re trying to fix (flex
items default to `min-height: auto`). Add `minH={0}` on `TabPanels` (or
otherwise ensure the scroll container is allowed to shrink) so scrolling is
reliably contained within the tab content across browsers.
```suggestion
<TabPanels flex={1} overflowY="auto" minH={0}>
```
##########
airflow/www/static/js/dag/details/index.tsx:
##########
@@ -491,15 +495,12 @@ const Details = ({
</TabPanel>
)}
{isDag && (
- <TabPanel height="100%" width="100%" overflow="auto">
+ <TabPanel p={0}>
Review Comment:
The Calendar tab previously set `overflow="auto"` on the `TabPanel`, but now
relies on `TabPanels` for scrolling. `Calendar` renders a fixed-width (900px)
and variable-height chart box, so without `overflowX` on the container users
may lose the ability to horizontally scroll to see the full chart. Consider
restoring horizontal overflow for the Calendar panel (e.g. `overflowX="auto"`)
or otherwise ensuring wide calendar content remains accessible.
```suggestion
<TabPanel p={0} overflowX="auto">
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
