feat(Ellipsis): add component#2690
Open
PahaN47 wants to merge 1 commit into
Open
Conversation
Contributor
Reviewer's GuideAdds a new Ellipsis React component that supports start/center/end truncation with optional segment-based offsets, using a ResizeObserver-driven hook for center-ellipsis measurement and updates theme flow variables to support direction-aware behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
useCenterEllipsis,measureRefis declared asReact.useRef<HTMLSpanElement>(null), which is incompatible withnullas the initial value under strict typing; consider changing it toReact.useRef<HTMLSpanElement | null>(null)(and adjusting usages) to avoid TypeScript errors. - The separator handling in
EllipsisusesArray.from(separator)and thenincludes(char)on each character of the text; this effectively only supports single-character separators even when an array is passed—if you intend to support multi-character separators, you may want to clarify the prop type or adjust the splitting logic to operate on substrings instead of individual characters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useCenterEllipsis`, `measureRef` is declared as `React.useRef<HTMLSpanElement>(null)`, which is incompatible with `null` as the initial value under strict typing; consider changing it to `React.useRef<HTMLSpanElement | null>(null)` (and adjusting usages) to avoid TypeScript errors.
- The separator handling in `Ellipsis` uses `Array.from(separator)` and then `includes(char)` on each character of the text; this effectively only supports single-character separators even when an array is passed—if you intend to support multi-character separators, you may want to clarify the prop type or adjust the splitting logic to operate on substrings instead of individual characters.
## Individual Comments
### Comment 1
<location path="src/components/Ellipsis/hooks.ts" line_range="156-162" />
<code_context>
+ );
+
+ // does the same as useLayoutEffect, but does not trigger warnings
+ const containerRefCallback = React.useCallback<React.RefCallback<HTMLSpanElement>>(
+ (node) => {
+ if (!node) {
+ return;
+ }
+
+ if (!containerRef.current) {
+ document.fonts.ready.then(() => handleTextChange(node));
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid double work and potential race between `document.fonts.ready` and the `useEffect` that also triggers `handleTextChange`.
Right now `handleTextChange` can run twice on initial mount (from `document.fonts.ready` and from the `useEffect` that runs on mount/text change), and `fonts.ready` can still resolve after unmount, calling `handleTextChange(node)` on a cleaned-up component.
Consider:
- Using a `mounted` ref and early-returning in the `fonts.ready` callback if unmounted, and/or
- Moving the `fonts.ready` logic into the same effect that does the initial measurement so only one path runs it.
This avoids duplicate work and prevents lifecycle races.
Suggested implementation:
```typescript
);
const mountedRef = React.useRef(false);
const initialMeasureDoneRef = React.useRef(false);
React.useEffect(() => {
mountedRef.current = true;
return () => {
mountedRef.current = false;
};
}, []);
// does the same as useLayoutEffect, but does not trigger warnings
const containerRefCallback = React.useCallback<React.RefCallback<HTMLSpanElement>>(
(node) => {
containerRef.current = node;
if (!node) {
return;
}
// Schedule initial measurement once fonts are ready, but only once
if (typeof document !== 'undefined' && 'fonts' in document) {
document.fonts.ready.then(() => {
if (!mountedRef.current || initialMeasureDoneRef.current) {
return;
}
initialMeasureDoneRef.current = true;
handleTextChange(node);
});
} else if (!initialMeasureDoneRef.current) {
initialMeasureDoneRef.current = true;
handleTextChange(node);
}
},
[handleTextChange],
);
const ELLIPSIS_CHAR = '\u2026';
```
To fully avoid duplicate work and races, update the `useEffect` that currently calls `handleTextChange` on mount/text change to guard with `initialMeasureDoneRef` as well. For example, wrap its `handleTextChange(container)` call with:
```ts
if (!initialMeasureDoneRef.current && container) {
initialMeasureDoneRef.current = true;
handleTextChange(container);
}
```
This ensures that either the `fonts.ready` path or the effect does the initial measure, but never both, and no calls happen after unmount.
</issue_to_address>
### Comment 2
<location path="src/components/Ellipsis/hooks.ts" line_range="13" />
<code_context>
+
+const ELLIPSIS_CHAR = '\u2026';
+
+let observer: ResizeObserver | null = null;
+const resizeParamsMap: Map<HTMLSpanElement, ResizeParams> = new Map();
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring this hook to use per-instance resize observers and a pure helper for computing visible text instead of shared globals and mixed concerns.
You can simplify this hook a lot by pushing the observer and resize state into the hook instance and extracting the pure “compute visible text” logic.
### 1. Remove global `observer`, `resizeParamsMap`, and `callId`
You only ever use a single container per hook instance. You can keep all needed params in refs and have a per-instance `ResizeObserver`. That removes the Map, bookkeeping helpers, and the re-entrancy guard.
**Instead of:**
```ts
let observer: ResizeObserver | null = null;
const resizeParamsMap: Map<HTMLSpanElement, ResizeParams> = new Map();
let callId = 0;
const observerCallback = async (entries: ResizeObserverEntry[]) => {
callId++;
const currentCallId = callId;
for (const {target} of entries) {
handleResize(target as HTMLSpanElement);
if (currentCallId !== callId) {
return;
}
}
};
const subscribeResize = (container: HTMLSpanElement | null) => { /* ... */ }
const unsubscribeResize = (container: HTMLSpanElement | null) => { /* ... */ }
const setResizeParams = (...) => { /* ... */ }
const deleteResizeParams = (...) => { /* ... */ }
```
**Use a per-hook setup:**
```ts
export const useCenterEllipsis = ({ text, startOffset = '', endOffset = '' }: CenterEllipsisProps) => {
const containerRef = React.useRef<HTMLSpanElement | null>(null);
const measureRef = React.useRef<HTMLSpanElement | null>(null);
const observerRef = React.useRef<ResizeObserver | null>(null);
const resizeParamsRef = React.useRef<ResizeParams>({
text,
startOffset,
endOffset,
setVisibleText: () => {},
measure: null,
});
const [visibleText, setVisibleText] = React.useState(text);
// keep params up-to-date
React.useEffect(() => {
resizeParamsRef.current = {
text,
startOffset,
endOffset,
setVisibleText,
measure: measureRef.current,
};
}, [text, startOffset, endOffset]);
const handleResize = React.useCallback(() => {
const container = containerRef.current;
const { text, startOffset, endOffset, setVisibleText, measure } = resizeParamsRef.current;
if (!container || !measure) return;
const next = computeVisibleText({ container, measure, text, startOffset, endOffset });
setVisibleText(next);
}, []);
React.useEffect(() => {
const container = containerRef.current;
if (!container) return;
const observer = new ResizeObserver(() => {
handleResize(); // no callId needed; idempotent
});
observerRef.current = observer;
observer.observe(container);
return () => {
observer.disconnect();
observerRef.current = null;
};
}, [handleResize]);
// ...
};
```
This keeps all lifecycle and state local to the hook and removes the need for `callId` and the global `Map`.
### 2. Extract a pure `computeVisibleText` function
`handleResize` currently mixes DOM reading with the binary-search ellipsis algorithm. Extract the algorithm into a pure helper that takes minimal inputs and returns the final string.
**Extract the core algorithm:**
```ts
interface ComputeVisibleTextArgs {
text: string;
startOffset: string;
endOffset: string;
measureWidth: (candidate: string) => number;
availableWidth: number;
}
const ELLIPSIS_CHAR = '\u2026';
const computeVisibleTextString = ({
text,
startOffset,
endOffset,
measureWidth,
availableWidth,
}: ComputeVisibleTextArgs): string => {
const collapsibleStartIndex = startOffset.length;
const collapsibleEndIndex = -endOffset.length || text.length;
const collapsibleText = text.slice(collapsibleStartIndex, collapsibleEndIndex);
// full text fits
if (measureWidth(text) <= availableWidth) {
return text;
}
let minCharacters = 0;
let maxCharacters = collapsibleText.length;
let result = startOffset + ELLIPSIS_CHAR + endOffset;
while (minCharacters <= maxCharacters) {
const currentLength = Math.floor((minCharacters + maxCharacters) * 0.5);
const start = collapsibleText.slice(0, Math.ceil(currentLength * 0.5));
const end = collapsibleText.slice(collapsibleText.length - Math.floor(currentLength * 0.5));
const candidate = startOffset + start + ELLIPSIS_CHAR + end + endOffset;
if (measureWidth(candidate) <= availableWidth) {
result = candidate;
minCharacters = currentLength + 1;
} else {
maxCharacters = currentLength - 1;
}
}
return result;
};
```
**Then `handleResize` becomes a thin wrapper:**
```ts
const computeVisibleText = ({
container,
measure,
text,
startOffset,
endOffset,
}: {
container: HTMLSpanElement;
measure: HTMLSpanElement;
text: string;
startOffset: string;
endOffset: string;
}) => {
const availableWidth = container.getBoundingClientRect().width;
if (availableWidth <= 0) return text;
const measureWidth = (candidate: string) => {
measure.textContent = candidate;
return measure.getBoundingClientRect().width;
};
return computeVisibleTextString({
text,
startOffset,
endOffset,
measureWidth,
availableWidth,
});
};
```
Now:
- All cross-instance coordination is gone.
- The resize logic is easier to reason about (no `callId`, no global `Map`).
- The core ellipsis algorithm is testable in isolation without DOM/React.
</issue_to_address>
### Comment 3
<location path="src/components/Ellipsis/Ellipsis.tsx" line_range="69" />
<code_context>
+
+ const isCenterPosition = position === 'center';
+
+ const [startOffset, ellipsis, endOffset] = React.useMemo<[string, string, string]>(() => {
+ const textLength = text.length;
+ if (!separator) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the offset calculation, edge-ellipsis rendering, and copy-handling logic into small helpers/hooks to make the Ellipsis component primarily simple wiring rather than dense inline logic.
You can keep the current feature set but reduce perceived complexity with a few small extractions and a clearer separator model.
### 1. Split the `useMemo` into small helpers + normalized separators
Right now the `useMemo` does:
- negative index handling
- separator/string[] handling (with `Array.from`)
- scanning from both ends
- slicing
You can make this more readable by:
- normalizing `separator` once
- delegating to small helpers for separator vs non‑separator logic
```ts
type Offsets = [string, string, string];
function computeOffsetsNoSeparator(
text: string,
offsetStart: number,
offsetEnd: number,
): Offsets {
const textLength = text.length;
const offsetEndLocal = -offsetEnd || textLength;
return [
text.slice(0, offsetStart),
text.slice(offsetStart, offsetEndLocal),
text.slice(offsetEndLocal),
];
}
function computeOffsetsWithSeparators(
text: string,
separators: string[],
offsetStart: number,
offsetEnd: number,
): Offsets {
const textLength = text.length;
let startPartsLeft = offsetStart;
let startOffsetEnd = 0;
let endPartsLeft = offsetEnd;
let endOffsetStart = textLength;
for (let i = 0; i < textLength && (startPartsLeft || endPartsLeft); i++) {
const charStart = text[i];
const charEnd = text[textLength - i - 1];
if (startPartsLeft && separators.includes(charStart)) {
startPartsLeft--;
startOffsetEnd = i;
}
if (endPartsLeft && separators.includes(charEnd)) {
endPartsLeft--;
endOffsetStart = textLength - i;
}
}
return [
text.slice(0, startOffsetEnd),
text.slice(startOffsetEnd, endOffsetStart),
text.slice(endOffsetStart),
];
}
```
Then inside the component:
```ts
const separators = React.useMemo(
() => (separator ? (Array.isArray(separator) ? separator : [separator]) : []),
[separator],
);
const [startOffset, ellipsis, endOffset] = React.useMemo<Offsets>(() => {
if (!separators.length) {
return computeOffsetsNoSeparator(text, offsetStart, offsetEnd);
}
return computeOffsetsWithSeparators(text, separators, offsetStart, offsetEnd);
}, [text, separators, offsetStart, offsetEnd]);
```
This keeps behavior intact but makes each piece much easier to follow.
### 2. Extract the non‑center render path
The conditional JSX for center vs edge ellipsis duplicates structure and mixes layout + special bidi handling. Extracting the non‑center branch into a tiny component or render function lets the main component stay “routing only”:
```ts
interface EdgeEllipsisProps {
position: EllipsisPosition;
startOffset: string;
ellipsis: string;
endOffset: string;
}
function EdgeEllipsis({position, startOffset, ellipsis, endOffset}: EdgeEllipsisProps) {
return (
<>
<span aria-hidden>{startOffset}</span>
<span className={b('ellipsis', {[position]: true})} aria-hidden>
<span className={b('ellipsis-content')}>{`${FSI}${ellipsis}${PDI}`}</span>
</span>
<span aria-hidden>{endOffset}</span>
</>
);
}
```
Usage:
```tsx
return (
<span
className={b(null, className)}
style={style}
ref={ref}
onCopy={handleCopy}
aria-label={text}
>
{isCenterPosition ? (
<CenterEllipsis
startOffset={startOffset}
endOffset={endOffset}
ref={ellipsisContentRef}
>
{text}
</CenterEllipsis>
) : (
<EdgeEllipsis
position={position}
startOffset={startOffset}
ellipsis={ellipsis}
endOffset={endOffset}
/>
)}
</span>
);
```
### 3. Move copy logic into a hook
The copy behavior is useful but cross‑cutting. Pulling it into a hook reduces noise in the component and localizes the clipboard logic:
```ts
function useRestoreFullTextOnCopy(
enabled: boolean,
text: string,
startOffset: string,
endOffset: string,
ellipsisContentRef: React.RefObject<HTMLSpanElement>,
) {
return React.useCallback(
(e: React.ClipboardEvent<HTMLSpanElement>) => {
if (!enabled) {
return;
}
const clipboardText = (window.getSelection()?.toString() || '').trim();
const currentText =
startOffset + (ellipsisContentRef.current?.textContent || '') + endOffset;
if (currentText === clipboardText) {
e.preventDefault();
e.clipboardData.setData('text/plain', text);
}
},
[enabled, text, startOffset, endOffset],
);
}
```
Then in `Ellipsis`:
```ts
const handleCopy = useRestoreFullTextOnCopy(
isCenterPosition,
text,
startOffset,
endOffset,
ellipsisContentRef,
);
```
These extractions keep all current behavior but make the main component mostly wiring, which directly addresses the “too much in one place” concern without changing semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+156
to
+162
| const containerRefCallback = React.useCallback<React.RefCallback<HTMLSpanElement>>( | ||
| (node) => { | ||
| if (!node) { | ||
| return; | ||
| } | ||
|
|
||
| if (!containerRef.current) { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Avoid double work and potential race between document.fonts.ready and the useEffect that also triggers handleTextChange.
Right now handleTextChange can run twice on initial mount (from document.fonts.ready and from the useEffect that runs on mount/text change), and fonts.ready can still resolve after unmount, calling handleTextChange(node) on a cleaned-up component.
Consider:
- Using a
mountedref and early-returning in thefonts.readycallback if unmounted, and/or - Moving the
fonts.readylogic into the same effect that does the initial measurement so only one path runs it.
This avoids duplicate work and prevents lifecycle races.
Suggested implementation:
);
const mountedRef = React.useRef(false);
const initialMeasureDoneRef = React.useRef(false);
React.useEffect(() => {
mountedRef.current = true;
return () => {
mountedRef.current = false;
};
}, []);
// does the same as useLayoutEffect, but does not trigger warnings
const containerRefCallback = React.useCallback<React.RefCallback<HTMLSpanElement>>(
(node) => {
containerRef.current = node;
if (!node) {
return;
}
// Schedule initial measurement once fonts are ready, but only once
if (typeof document !== 'undefined' && 'fonts' in document) {
document.fonts.ready.then(() => {
if (!mountedRef.current || initialMeasureDoneRef.current) {
return;
}
initialMeasureDoneRef.current = true;
handleTextChange(node);
});
} else if (!initialMeasureDoneRef.current) {
initialMeasureDoneRef.current = true;
handleTextChange(node);
}
},
[handleTextChange],
);
const ELLIPSIS_CHAR = '\u2026';To fully avoid duplicate work and races, update the useEffect that currently calls handleTextChange on mount/text change to guard with initialMeasureDoneRef as well. For example, wrap its handleTextChange(container) call with:
if (!initialMeasureDoneRef.current && container) {
initialMeasureDoneRef.current = true;
handleTextChange(container);
}This ensures that either the fonts.ready path or the effect does the initial measure, but never both, and no calls happen after unmount.
|
|
||
| const ELLIPSIS_CHAR = '\u2026'; | ||
|
|
||
| let observer: ResizeObserver | null = null; |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring this hook to use per-instance resize observers and a pure helper for computing visible text instead of shared globals and mixed concerns.
You can simplify this hook a lot by pushing the observer and resize state into the hook instance and extracting the pure “compute visible text” logic.
1. Remove global observer, resizeParamsMap, and callId
You only ever use a single container per hook instance. You can keep all needed params in refs and have a per-instance ResizeObserver. That removes the Map, bookkeeping helpers, and the re-entrancy guard.
Instead of:
let observer: ResizeObserver | null = null;
const resizeParamsMap: Map<HTMLSpanElement, ResizeParams> = new Map();
let callId = 0;
const observerCallback = async (entries: ResizeObserverEntry[]) => {
callId++;
const currentCallId = callId;
for (const {target} of entries) {
handleResize(target as HTMLSpanElement);
if (currentCallId !== callId) {
return;
}
}
};
const subscribeResize = (container: HTMLSpanElement | null) => { /* ... */ }
const unsubscribeResize = (container: HTMLSpanElement | null) => { /* ... */ }
const setResizeParams = (...) => { /* ... */ }
const deleteResizeParams = (...) => { /* ... */ }Use a per-hook setup:
export const useCenterEllipsis = ({ text, startOffset = '', endOffset = '' }: CenterEllipsisProps) => {
const containerRef = React.useRef<HTMLSpanElement | null>(null);
const measureRef = React.useRef<HTMLSpanElement | null>(null);
const observerRef = React.useRef<ResizeObserver | null>(null);
const resizeParamsRef = React.useRef<ResizeParams>({
text,
startOffset,
endOffset,
setVisibleText: () => {},
measure: null,
});
const [visibleText, setVisibleText] = React.useState(text);
// keep params up-to-date
React.useEffect(() => {
resizeParamsRef.current = {
text,
startOffset,
endOffset,
setVisibleText,
measure: measureRef.current,
};
}, [text, startOffset, endOffset]);
const handleResize = React.useCallback(() => {
const container = containerRef.current;
const { text, startOffset, endOffset, setVisibleText, measure } = resizeParamsRef.current;
if (!container || !measure) return;
const next = computeVisibleText({ container, measure, text, startOffset, endOffset });
setVisibleText(next);
}, []);
React.useEffect(() => {
const container = containerRef.current;
if (!container) return;
const observer = new ResizeObserver(() => {
handleResize(); // no callId needed; idempotent
});
observerRef.current = observer;
observer.observe(container);
return () => {
observer.disconnect();
observerRef.current = null;
};
}, [handleResize]);
// ...
};This keeps all lifecycle and state local to the hook and removes the need for callId and the global Map.
2. Extract a pure computeVisibleText function
handleResize currently mixes DOM reading with the binary-search ellipsis algorithm. Extract the algorithm into a pure helper that takes minimal inputs and returns the final string.
Extract the core algorithm:
interface ComputeVisibleTextArgs {
text: string;
startOffset: string;
endOffset: string;
measureWidth: (candidate: string) => number;
availableWidth: number;
}
const ELLIPSIS_CHAR = '\u2026';
const computeVisibleTextString = ({
text,
startOffset,
endOffset,
measureWidth,
availableWidth,
}: ComputeVisibleTextArgs): string => {
const collapsibleStartIndex = startOffset.length;
const collapsibleEndIndex = -endOffset.length || text.length;
const collapsibleText = text.slice(collapsibleStartIndex, collapsibleEndIndex);
// full text fits
if (measureWidth(text) <= availableWidth) {
return text;
}
let minCharacters = 0;
let maxCharacters = collapsibleText.length;
let result = startOffset + ELLIPSIS_CHAR + endOffset;
while (minCharacters <= maxCharacters) {
const currentLength = Math.floor((minCharacters + maxCharacters) * 0.5);
const start = collapsibleText.slice(0, Math.ceil(currentLength * 0.5));
const end = collapsibleText.slice(collapsibleText.length - Math.floor(currentLength * 0.5));
const candidate = startOffset + start + ELLIPSIS_CHAR + end + endOffset;
if (measureWidth(candidate) <= availableWidth) {
result = candidate;
minCharacters = currentLength + 1;
} else {
maxCharacters = currentLength - 1;
}
}
return result;
};Then handleResize becomes a thin wrapper:
const computeVisibleText = ({
container,
measure,
text,
startOffset,
endOffset,
}: {
container: HTMLSpanElement;
measure: HTMLSpanElement;
text: string;
startOffset: string;
endOffset: string;
}) => {
const availableWidth = container.getBoundingClientRect().width;
if (availableWidth <= 0) return text;
const measureWidth = (candidate: string) => {
measure.textContent = candidate;
return measure.getBoundingClientRect().width;
};
return computeVisibleTextString({
text,
startOffset,
endOffset,
measureWidth,
availableWidth,
});
};Now:
- All cross-instance coordination is gone.
- The resize logic is easier to reason about (no
callId, no globalMap). - The core ellipsis algorithm is testable in isolation without DOM/React.
|
|
||
| const isCenterPosition = position === 'center'; | ||
|
|
||
| const [startOffset, ellipsis, endOffset] = React.useMemo<[string, string, string]>(() => { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the offset calculation, edge-ellipsis rendering, and copy-handling logic into small helpers/hooks to make the Ellipsis component primarily simple wiring rather than dense inline logic.
You can keep the current feature set but reduce perceived complexity with a few small extractions and a clearer separator model.
1. Split the useMemo into small helpers + normalized separators
Right now the useMemo does:
- negative index handling
- separator/string[] handling (with
Array.from) - scanning from both ends
- slicing
You can make this more readable by:
- normalizing
separatoronce - delegating to small helpers for separator vs non‑separator logic
type Offsets = [string, string, string];
function computeOffsetsNoSeparator(
text: string,
offsetStart: number,
offsetEnd: number,
): Offsets {
const textLength = text.length;
const offsetEndLocal = -offsetEnd || textLength;
return [
text.slice(0, offsetStart),
text.slice(offsetStart, offsetEndLocal),
text.slice(offsetEndLocal),
];
}
function computeOffsetsWithSeparators(
text: string,
separators: string[],
offsetStart: number,
offsetEnd: number,
): Offsets {
const textLength = text.length;
let startPartsLeft = offsetStart;
let startOffsetEnd = 0;
let endPartsLeft = offsetEnd;
let endOffsetStart = textLength;
for (let i = 0; i < textLength && (startPartsLeft || endPartsLeft); i++) {
const charStart = text[i];
const charEnd = text[textLength - i - 1];
if (startPartsLeft && separators.includes(charStart)) {
startPartsLeft--;
startOffsetEnd = i;
}
if (endPartsLeft && separators.includes(charEnd)) {
endPartsLeft--;
endOffsetStart = textLength - i;
}
}
return [
text.slice(0, startOffsetEnd),
text.slice(startOffsetEnd, endOffsetStart),
text.slice(endOffsetStart),
];
}Then inside the component:
const separators = React.useMemo(
() => (separator ? (Array.isArray(separator) ? separator : [separator]) : []),
[separator],
);
const [startOffset, ellipsis, endOffset] = React.useMemo<Offsets>(() => {
if (!separators.length) {
return computeOffsetsNoSeparator(text, offsetStart, offsetEnd);
}
return computeOffsetsWithSeparators(text, separators, offsetStart, offsetEnd);
}, [text, separators, offsetStart, offsetEnd]);This keeps behavior intact but makes each piece much easier to follow.
2. Extract the non‑center render path
The conditional JSX for center vs edge ellipsis duplicates structure and mixes layout + special bidi handling. Extracting the non‑center branch into a tiny component or render function lets the main component stay “routing only”:
interface EdgeEllipsisProps {
position: EllipsisPosition;
startOffset: string;
ellipsis: string;
endOffset: string;
}
function EdgeEllipsis({position, startOffset, ellipsis, endOffset}: EdgeEllipsisProps) {
return (
<>
<span aria-hidden>{startOffset}</span>
<span className={b('ellipsis', {[position]: true})} aria-hidden>
<span className={b('ellipsis-content')}>{`${FSI}${ellipsis}${PDI}`}</span>
</span>
<span aria-hidden>{endOffset}</span>
</>
);
}Usage:
return (
<span
className={b(null, className)}
style={style}
ref={ref}
onCopy={handleCopy}
aria-label={text}
>
{isCenterPosition ? (
<CenterEllipsis
startOffset={startOffset}
endOffset={endOffset}
ref={ellipsisContentRef}
>
{text}
</CenterEllipsis>
) : (
<EdgeEllipsis
position={position}
startOffset={startOffset}
ellipsis={ellipsis}
endOffset={endOffset}
/>
)}
</span>
);3. Move copy logic into a hook
The copy behavior is useful but cross‑cutting. Pulling it into a hook reduces noise in the component and localizes the clipboard logic:
function useRestoreFullTextOnCopy(
enabled: boolean,
text: string,
startOffset: string,
endOffset: string,
ellipsisContentRef: React.RefObject<HTMLSpanElement>,
) {
return React.useCallback(
(e: React.ClipboardEvent<HTMLSpanElement>) => {
if (!enabled) {
return;
}
const clipboardText = (window.getSelection()?.toString() || '').trim();
const currentText =
startOffset + (ellipsisContentRef.current?.textContent || '') + endOffset;
if (currentText === clipboardText) {
e.preventDefault();
e.clipboardData.setData('text/plain', text);
}
},
[enabled, text, startOffset, endOffset],
);
}Then in Ellipsis:
const handleCopy = useRestoreFullTextOnCopy(
isCenterPosition,
text,
startOffset,
endOffset,
ellipsisContentRef,
);These extractions keep all current behavior but make the main component mostly wiring, which directly addresses the “too much in one place” concern without changing semantics.
Contributor
|
Preview is ready. |
Contributor
|
🎭 Component Tests Report is ready. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a reusable Ellipsis text component with configurable truncation behavior and RTL-safe styling, including stories and theme support for flow direction.
New Features:
Enhancements: