Skip to content

feat: add pageContainer header slots#39

Merged
hibig merged 5 commits into
mainfrom
dev
Jul 1, 2026
Merged

feat: add pageContainer header slots#39
hibig merged 5 commits into
mainfrom
dev

Conversation

@hibig

@hibig hibig commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

hibig added 5 commits June 24, 2026 18:49
…erInner

Add HeaderSlotContext/HeaderLeft/HeaderRight/usePageContentStyle so the host and enterprise plugin resolve one context instance and portal into the same header bar. Remove the unused prop-based PageContainerInner/PageBox and its CSS.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the page container components by moving host-specific layout concerns out of the core UI package and introducing shared header-slot primitives (HeaderLeft, HeaderRight, and usePageContentStyle) to support cross-package portaling. It also refines dropdown button click handling, select wrapper styles, and table header sorter styles. Feedback highlights a potential bug where spreading item.props could re-introduce an omitted onClick handler, opportunities to optimize useEffect dependencies in the header slots, an ESLint exhaustive-deps warning in usePageContentStyle, and a CSS selector issue with active sorter icons on hover.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +126 to 127
..._.omit(item, ['label', 'locale', 'onClick']),
...item.props,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If item.props contains onClick, spreading ...item.props after _.omit(item, ['label', 'locale', 'onClick']) will overwrite the omission and re-introduce onClick into the menu item. This would bypass the intended omission and cause duplicate execution of the click handler.

Consider omitting onClick from item.props as well to ensure it is completely removed.

Suggested change
..._.omit(item, ['label', 'locale', 'onClick']),
...item.props,
..._.omit(item, ['label', 'locale', 'onClick', 'props']),
..._.omit(item.props, ['onClick']),

Comment on lines +34 to +35
const ctx = useContext(HeaderSlotContext);
useEffect(() => ctx?.registerSlot('left'), [ctx]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Depending on the entire ctx object in the dependency array of useEffect means this effect will re-run and re-register the slot whenever any property of the context changes (such as rightEl or leftEl changing). This can cause unnecessary registration/deregistration cycles.

Consider extracting registerSlot from ctx and depending only on it.

  const ctx = useContext(HeaderSlotContext);
  const registerSlot = ctx?.registerSlot;
  useEffect(() => {
    return registerSlot?.('left');
  }, [registerSlot]);

Comment on lines +42 to +43
const ctx = useContext(HeaderSlotContext);
useEffect(() => ctx?.registerSlot('right'), [ctx]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Depending on the entire ctx object in the dependency array of useEffect means this effect will re-run and re-register the slot whenever any property of the context changes (such as leftEl or rightEl changing). This can cause unnecessary registration/deregistration cycles.

Consider extracting registerSlot from ctx and depending only on it.

  const ctx = useContext(HeaderSlotContext);
  const registerSlot = ctx?.registerSlot;
  useEffect(() => {
    return registerSlot?.('right');
  }, [registerSlot]);

Comment on lines +50 to +57
export const usePageContentStyle = (style?: React.CSSProperties): void => {
const ctx = useContext(HeaderSlotContext);
const stable = JSON.stringify(style ?? null);
useLayoutEffect(() => {
if (!ctx) return;
return ctx.setContentStyle(style);
}, [stable, ctx?.setContentStyle]);
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In usePageContentStyle, referencing style inside useLayoutEffect without including it in the dependency array violates the react-hooks/exhaustive-deps rule. However, including style directly would cause the effect to run on every render if a new object reference is passed.

To resolve this cleanly without ESLint warnings or unnecessary re-runs, you can parse the stable string back to an object inside the effect.

Suggested change
export const usePageContentStyle = (style?: React.CSSProperties): void => {
const ctx = useContext(HeaderSlotContext);
const stable = JSON.stringify(style ?? null);
useLayoutEffect(() => {
if (!ctx) return;
return ctx.setContentStyle(style);
}, [stable, ctx?.setContentStyle]);
};
export const usePageContentStyle = (style?: React.CSSProperties): void => {
const ctx = useContext(HeaderSlotContext);
const setContentStyle = ctx?.setContentStyle;
const stable = JSON.stringify(style ?? null);
useLayoutEffect(() => {
if (!setContentStyle) return;
const parsedStyle = stable !== 'null' ? JSON.parse(stable) : undefined;
return setContentStyle(parsedStyle);
}, [stable, setContentStyle]);
};

Comment on lines +45 to 47
.anticon:not(.sorter-active) {
color: var(--ant-table-header-icon-hover-color);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since .sorter-active is applied to a parent/wrapper element of .anticon rather than the .anticon element itself (as shown by the nested rule .sorter-active { .anticon { ... } }), the selector .anticon:not(.sorter-active) will still match the active icon on hover, causing its color to be overridden by the hover color.

Consider explicitly preserving the active color for .sorter-active .anticon on hover.

        .anticon {
          color: var(--ant-table-header-icon-hover-color);
        }
        .sorter-active .anticon {
          color: var(--ant-color-primary);
        }

@hibig hibig merged commit 732f6af into main Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant