Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/ssr full display all invisibly #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix/ssr full display all invisibly #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8998234
fix: make srr="full" render full items invisible initially
huan-qiu Oct 14, 2023
0c7a755
test: update snapshot of ssr test case
huan-qiu Oct 14, 2023
411fb3b
fix: make changes of ssr full only affect code that set to be responsive
huan-qiu Oct 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions src/Overflow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,19 @@ function Overflow<ItemType = any>(
// ================================ Render ================================
const displayRest = restReady && !!omittedItems.length;

let suffixStyle: React.CSSProperties = {};
const isFullySSRResponsiveFirstRender =
fullySSR && shouldResponsive && containerWidth === null;
const fullySSRFirstRenderStyle: React.CSSProperties = {
Copy link
Member

Choose a reason for hiding this comment

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

有点蛋疼，效果感觉也不好。SSR 的时候这块就直接空了也会有 CLS。如果是 SSR Full Render 然后 SSR 的时候 overflow hidden 掉呢？

Copy link
Author

Choose a reason for hiding this comment

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

在overflow没设置高度下，会有一点shift，因为高度被撑大了。如果有设置的话，就是先空白然后显示，layout上就不会shift。如果是 SSR Full Render + overflow hidden，假设能让 item 是都显示在同一行溢出的话，效果是后面两个item的位置可能会闪一下。这个的前提是要 overflow 这个组件不要 wrap 成多行，目前是会wrap成多行，需要先有高度限制然后来 x y 都overflow hidden。看似都有 tradeoff。

Copy link
Author

Choose a reason for hiding this comment

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

@zombieJ overflow组件 有接受个style的prop，因此在pr改动的基础上，可以传入height的值来先占好需要的高度。我理解这样做在 fullySSR 就不会有 CLS 的问题。因为 fullySSR 这个问题目前只会影响到 overflow 是水平一行 maxCount 值是RESPONSIVE的时候。看了下 rc-menu 使用 rc-overflow的时候是会传入 style prop的，rc-select 则是没开启 ssr，所以没有这个问题。你康康ok不？

Copy link
Author

Choose a reason for hiding this comment

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

@zombieJ 如果暂时没有满意的方案，可以先让ssr=full在 menu 那里可以 opt out 吗？即考虑这条 pr react-component/menu#665

maxWidth: 0,
padding: 0,
margin: 0,
borderWidth: 0,
overflowX: 'hidden',
};

let suffixStyle: React.CSSProperties = isFullySSRResponsiveFirstRender
? fullySSRFirstRenderStyle
: {};
if (suffixFixedStart !== null && shouldResponsive) {
suffixStyle = {
position: 'absolute',
Expand All @@ -316,13 +328,22 @@ function Overflow<ItemType = any>(
responsive: shouldResponsive,
component: itemComponent,
invalidate,
style: isFullySSRResponsiveFirstRender
? fullySSRFirstRenderStyle
: undefined,
};

// >>>>> Choice render fun by `renderRawItem`
const internalRenderItemNode = renderRawItem
? (item: ItemType, index: number) => {
const key = getKey(item, index);

const isIdxCheckPass = index <= mergedDisplayCount;
// in `ssr="full"` case, item's `display` will be set to `true` when either condition is met:
// 1) at initial render; 2) its corresponding width is valid and pass the index check
const shouldDisplay = fullySSR
? isFullySSRResponsiveFirstRender ||
(isIdxCheckPass && getItemWidth(index) > 0)
: isIdxCheckPass;
return (
<OverflowContext.Provider
key={key}
Expand All @@ -332,7 +353,7 @@ function Overflow<ItemType = any>(
item,
itemKey: key,
registerSize,
display: index <= mergedDisplayCount,
display: shouldDisplay,
}}
>
{renderRawItem(item, index)}
Expand All @@ -341,7 +362,13 @@ function Overflow<ItemType = any>(
}
: (item: ItemType, index: number) => {
const key = getKey(item, index);

const isIdxCheckPass = index <= mergedDisplayCount;
// in `ssr="full"` case, item's `display` will be set to `true` when either condition is met:
// 1) at initial render; 2) its corresponding width is valid and pass the index check
const shouldDisplay = fullySSR
? isFullySSRResponsiveFirstRender ||
(isIdxCheckPass && getItemWidth(index) > 0)
: isIdxCheckPass;
return (
<Item
{...itemSharedProps}
Expand All @@ -351,7 +378,7 @@ function Overflow<ItemType = any>(
renderItem={mergedRenderItem}
itemKey={key}
registerSize={registerSize}
display={index <= mergedDisplayCount}
display={shouldDisplay}
/>
);
};
Expand Down
6 changes: 3 additions & 3 deletions tests/__snapshots__/ssr.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@ exports[`Overflow.SSR basic 1`] = `
>
<div
class="rc-overflow-item"
style="opacity:1;order:0"
style="opacity:1;order:0;max-width:0;padding:0;margin:0;border-width:0;overflow-x:hidden"
>
Label 0
</div>
<div
class="rc-overflow-item"
style="opacity:1;order:1"
style="opacity:1;order:1;max-width:0;padding:0;margin:0;border-width:0;overflow-x:hidden"
>
Label 1
</div>
<div
aria-hidden="true"
class="rc-overflow-item rc-overflow-item-rest"
style="opacity:0;height:0;overflow-y:hidden;order:9007199254740991;pointer-events:none;position:absolute"
style="opacity:0;height:0;overflow-y:hidden;order:9007199254740991;pointer-events:none;position:absolute;max-width:0;padding:0;margin:0;border-width:0;overflow-x:hidden"
>
+ 0 ...
</div>
Expand Down