aminghadersohi commented on code in PR #43632:
URL: https://github.com/apache/superset/pull/43632#discussion_r3904249578
##########
superset-frontend/src/explore/components/controls/DateFilterControl/tests/DateFilterLabel.test.tsx:
##########
@@ -123,19 +140,41 @@ test('DateFilter popover should attach to document.body
when not overflowing', (
userEvent.click(screen.getByText(NO_TIME_RANGE));
- const popover = document.querySelector('.time-range-popover');
+ const popover = document.querySelector<HTMLElement>('.time-range-popover');
expect(popover?.parentElement).toBe(document.body);
+ expect(popover).toHaveStyle({
+ width: 'min(600px, calc(100vw - 32px))',
+ });
+});
+
+test('DateFilter popover shifts into the viewport', async () => {
+ render(setup());
+
+ userEvent.click(screen.getByText(NO_TIME_RANGE));
+
+ await waitFor(() => {
+ expect(mockPopoverProps).toEqual(
+ expect.arrayContaining([
+ expect.objectContaining({
+ autoAdjustOverflow: SHIFT_INTO_VIEWPORT,
Review Comment:
NIT, optional. This asserts the value that only applies to *non*-cardinal
placements. `getAutoAdjustOverflow` returns `true` for
`top`/`bottom`/`left`/`right` and `SHIFT_INTO_VIEWPORT` for everything else
(`ControlPopover.tsx:80-81`), and `placement="right"` is passed in — so the
reason this reads `SHIFT_INTO_VIEWPORT` is that jsdom's zeroed
`getBoundingClientRect` sends `calculatePlacement` down the `xRatio < 0.35` /
`yRatio < 0.35` path to `rightTop`. The test passing is itself the evidence,
since a `right` placement would have resolved to `true`.
It does discriminate the thing I asked for — re-adding
`autoAdjustOverflow={false}` reds it, I checked. The brittleness is that a
trigger mid-viewport resolves to `left` and therefore `true`, which is equally
correct flip/shift behaviour that this assertion would reject, and adding
`rightTop` to `SHIFTING_PLACEMENTS` would break it for an unrelated reason.
`expect(...).toEqual(expect.objectContaining({ autoAdjustOverflow:
expect.not.falsy }))` isn't a thing, but asserting the recorded `placement`
alongside the value, or just that it is not `false`, would pin the intent
without pinning jsdom's layout.
##########
superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx:
##########
@@ -31,13 +31,9 @@ export interface BaseModalBodyProps {
}
export const BaseModalWrapper = styled(StyledModal)<BaseModalWrapperProps>`
- min-width: ${MIN_WIDTH}px;
- width: ${({ expanded }) => (expanded ? '100%' : MIN_WIDTH)} !important;
-
- @media (max-width: ${MIN_WIDTH + MODAL_MARGIN * 2}px) {
- width: 100% !important;
- min-width: auto;
- }
+ width: ${({ expanded }) =>
+ expanded ? '100%' : `${MODAL_WIDTH}px`} !important;
Review Comment:
NIT, optional. This pins the width where master let it grow. Master had
`min-width: 880px` with antd's `width: auto` on an `inline-block`
(`.ant-modal-centered .ant-modal`), i.e. shrink-to-fit with an 880 floor; this
is a hard 880. With normal content the two are identical at every width I
measured, so this is not a live regression. But with a child wider than 880 the
replica diverges:
| intrinsic-1400px child | master | this head |
|---|---|---|
| @1600x900 | 1568 | 880 |
| @1280x800 | 1248 | 880 |
Since `.ant-modal-body` is `overflow: hidden` (line 42), anything that did
exceed 880 would now be clipped rather than widen the modal. I could not find
content in this modal with an intrinsic width over 880 — the form items are
260px/49% — so I think this is fine and probably the behaviour you actually
intended, given the old unitless `880` was invalid and never applied. Flagging
it only so the trade is a choice rather than a side effect; `min-width`
alongside the `max-width` would preserve the old growth if you'd rather keep it.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
