amaannawab923 commented on code in PR #42614:
URL: https://github.com/apache/superset/pull/42614#discussion_r3684329678
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx:
##########
@@ -354,58 +425,59 @@ function AnnotationLayer({
const fetchSliceData = useCallback((id: string | number): void => {
const queryParams = rison.encode({
- columns: ['query_context'],
+ columns: ['params', 'query_context'],
});
SupersetClient.get({
endpoint: `/api/v1/chart/${id}?q=${queryParams}`,
- }).then(({ json }) => {
- const { result } = json;
- const queryContext = result.query_context;
- const formData = JSON.parse(queryContext).form_data;
- const dataObject = {
- data: {
- ...formData,
- groupby: formData.groupby?.map((column: QueryFormColumn) =>
- getColumnLabel(column),
- ),
- },
- };
- setSlice(dataObject);
- });
+ })
+ .then(({ json }) => {
+ const formData = getSliceFormData(json.result);
Review Comment:
minor, both fetchSliceData and fetchAppliedChart do the same
getSliceFormData -> toSliceData -> warn/catch dance now. could probably fold
that into one small helper so they don't drift, but honestly fine to leave it.
##########
superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx:
##########
@@ -48,12 +49,31 @@ export type PopoverProps = BasePopoverProps & {
getVisibilityRatio?: typeof getElementVisibilityRatio;
};
+/** Placements antd already shifts, capped so the arrow keeps touching its
trigger. */
+const SHIFTING_PLACEMENTS = new Set(['top', 'bottom', 'left', 'right']);
+
+// `shiftX`/`shiftY` work but are missing from `AdjustOverflow`, hence the
cast.
+export const SHIFT_INTO_VIEWPORT = {
+ adjustX: 1,
+ adjustY: 1,
+ shiftX: true,
+ shiftY: true,
+} as unknown as BasePopoverProps['autoAdjustOverflow'];
+
+// The other placements can flip a popup across its trigger but never nudge it
back
+// into the viewport, leaving an oversized one stranded off screen. Only they
opt in:
+// lifting the cap above would let those popups slide off a trigger scrolled
out of
+// view, taking the arrow away from what it points at.
+export const getAutoAdjustOverflow = (placement: TooltipPlacement) =>
Review Comment:
just flagging that this touches every ControlPopover, not just the
annotation modal. all the corner-placement ones pick it up too (adhoc metric,
adhoc filter, column select, date filter, contour, color breakpoints, time
series column). i think shifting into the viewport is the right call so no
objection, but it's a bit wider than the title lets on. worth clicking through
the adhoc metric/filter popovers to make sure nothing jumps weirdly, and maybe
mention in the description that it's a global change.
##########
superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx:
##########
@@ -48,12 +49,31 @@ export type PopoverProps = BasePopoverProps & {
getVisibilityRatio?: typeof getElementVisibilityRatio;
};
+/** Placements antd already shifts, capped so the arrow keeps touching its
trigger. */
+const SHIFTING_PLACEMENTS = new Set(['top', 'bottom', 'left', 'right']);
+
+// `shiftX`/`shiftY` work but are missing from `AdjustOverflow`, hence the
cast.
+export const SHIFT_INTO_VIEWPORT = {
+ adjustX: 1,
+ adjustY: 1,
+ shiftX: true,
+ shiftY: true,
+} as unknown as BasePopoverProps['autoAdjustOverflow'];
Review Comment:
tiny nit, the `as unknown as` basically throws the type away. shiftX/shiftY
are real rc-trigger options that antd just doesn't type, so it works, but it'll
silently break if antd ever changes that type. could extend the type locally
with shiftX/shiftY instead, or drop a link to the rc-trigger option in the
comment so it's traceable.
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx:
##########
@@ -1010,6 +1082,66 @@ function AnnotationLayer({
theme,
]);
+ const sliceConfiguration = renderSliceConfiguration();
+ const hasSliceConfiguration = !!sliceConfiguration;
+
+ const sectionGap = theme.sizeUnit * SECTION_GAP_UNITS;
+ const viewportInset = theme.sizeUnit * VIEWPORT_INSET_UNITS;
+
+ const measureSectionsMaxWidth = useCallback(() => {
+ const row = sectionsRef.current;
+ const popover = row?.closest('.ant-popover');
+ const panel = document.getElementById(CONTROL_SECTIONS_ID);
Review Comment:
this reaches out to the explore panel by id and measures a bunch of
getBoundingClientRects every tick, which is fine but a couple of things stood
out. it might get a little janky while you're dragging the panel resizer, and
if this control ever gets used outside normal explore it'll just quietly stop
measuring. not asking you to rewrite it, but a quick comment about the
explore-panel dependency would help whoever hits this next. also curious, did a
pure css cap (max-width with min/clamp) not work here or was it the popover
width settling that got in the way?
--
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]
