feat(Link): update link#1154
Open
ReFFaT wants to merge 3 commits into
Open
Conversation
Reviewer's GuideRefactors link creation/editing UX by replacing the old form-based link component with compact inline link inputs (for both tooltip and placeholder popup), adds a reusable Link form component with URL/text handling, and moves link creation/cancellation logic from the form into the surrounding editor plugins/widgets, including new behaviors for outside clicks, escape, undo/redo, and text saving. 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}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
SelectionTooltip, clicking the unlink button callsremoveLink, but the subsequentonOpenChange→onOutsideClickpath will still usecurrentUrland can immediately recreate the link; consider short‑circuitingonOpenChangewhenmanualHiddenis true or when the close was triggered by the unlink button. - The
.g-popup_openselector inTooltipView.scssglobally changes the border radius for all popups; if the intention is to style only the link tooltip, consider scoping this rule to the tooltip popup (e.g., via a more specific container class) to avoid affecting unrelated popups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SelectionTooltip`, clicking the unlink button calls `removeLink`, but the subsequent `onOpenChange` → `onOutsideClick` path will still use `currentUrl` and can immediately recreate the link; consider short‑circuiting `onOpenChange` when `manualHidden` is true or when the close was triggered by the unlink button.
- The `.g-popup_open` selector in `TooltipView.scss` globally changes the border radius for all popups; if the intention is to style only the link tooltip, consider scoping this rule to the tooltip popup (e.g., via a more specific container class) to avoid affecting unrelated popups.
## Individual Comments
### Comment 1
<location path="packages/editor/src/extensions/markdown/Link/plugins/LinkTooltipPlugin/TooltipView.tsx" line_range="36-38" />
<code_context>
- const inputEnterKeyHandler: TextInputProps['onKeyPress'] = enterKeyHandler(handleSubmit);
+ const inputEnterKeyHandler: TextInputProps['onKeyDown'] = enterKeyHandler(handleSubmit);
+
+ useEffect(() => {
+ onUrlChange?.(href);
+ }, [href, onUrlChange]);
return (
</code_context>
<issue_to_address>
**issue (bug_risk):** The local `url` state is not updated when `href` changes, which can desync the input from the selected link.
`useState(href)` only uses `href` as the initial value. If this tooltip is reused for another link or attrs change, `href` will update but the input will still show the old URL. Also update `url` when `href` changes, e.g.
```ts
useEffect(() => {
setUrl(href);
onUrlChange?.(href);
}, [href, onUrlChange]);
```
</issue_to_address>
### Comment 2
<location path="packages/editor/src/extensions/markdown/Link/plugins/LinkTooltipPlugin/TooltipView.tsx" line_range="60-64" />
<code_context>
+ </ActionTooltip>
+ )}
+ <ActionTooltip title={i18n('link_open_help')}>
+ <Button className={b('button')} view="flat" size="m" href={url} target="_blank">
+ <Icon data={LinkIcon} size={16} />
+ </Button>
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Opening links in a new tab should include `rel` attributes to avoid `window.opener` security issues.
Because this renders an anchor with `target="_blank"`, please also add `rel="noopener noreferrer"` (or confirm the UI kit applies it automatically) to prevent `window.opener` abuse:
```tsx
<Button
className={b('button')}
view="flat"
size="m"
href={url}
target="_blank"
rel="noopener noreferrer"
>
<Icon data={LinkIcon} size={16} />
</Button>
```
```suggestion
<ActionTooltip title={i18n('link_open_help')}>
<Button
className={b('button')}
view="flat"
size="m"
href={url}
target="_blank"
rel="noopener noreferrer"
>
<Icon data={LinkIcon} size={16} />
</Button>
</ActionTooltip>
```
</issue_to_address>
### Comment 3
<location path="packages/editor/src/extensions/markdown/Link/plugins/LinkTooltipPlugin/index.tsx" line_range="195-185" />
<code_context>
+ }
+
+ const url = currentUrlRef.current.trim();
+ if (url) {
+ onSubmit({url, text: currentTextRef.current});
+ } else {
</code_context>
<issue_to_address>
**suggestion:** `removeLink` and `onOutsideClick` both hide the tooltip and set `manualHidden`, causing redundant calls.
When `url` is empty, `onOutsideClick` calls `removeLink()`, which already sets `manualHidden = true` and calls `hideTooltip()`, and then `onOutsideClick` repeats both. While harmless, it’s confusing. Consider either:
- Letting `onOutsideClick` only choose between `changeAttrs` and `removeLink`, and have those handle hiding, or
- Centralizing all hiding/manual state in `onOutsideClick` and keeping `removeLink` focused on document changes.
Suggested implementation:
```typescript
private onOutsideClick = () => {
const url = this.currentUrl.trim();
if (!url) {
this.removeLink();
return;
}
const target = (event as MouseEvent | undefined)?.target as HTMLElement | null;
const isUndoRedoClick = Boolean(
target?.closest('[data-toolbar-item="undo"], [data-toolbar-item="redo"]'),
);
```
This change ensures that when `url` is empty, `onOutsideClick` delegates to `removeLink()` and returns early, so `manualHidden` and `hideTooltip` are not also called in `onOutsideClick`. This removes the redundant behavior specifically in the empty-URL path that you highlighted, assuming `removeLink()` already handles hiding and `manualHidden`. No further changes are required unless `removeLink()` currently does *not* manage those responsibilities, in which case that method should be updated accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
|
|
ReFFaT
force-pushed
the
feat-redisign-link
branch
from
9a66a32 to
9379f4c
Compare
ReFFaT
force-pushed
the
feat-redisign-link
branch
from
282ffc3 to
d908949
Compare
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.
refactor(link): rework link creation popup UX
Redesigned the link creation popup (triggered from cursor position) to match the existing link edit tooltip UX.
Changes:
LinkForm.tsx— removed Submit/Cancel buttons; replaced with two compact inline inputs (URL with clear + open button, and text with clear); addedonUrlChange/onTextChangecallbacksLinkForm.scss— new stylesheet matchingTooltipView.scssstylePlaceholderWidget/widget.tsx— moved submit logic from form to widget: outside click with a filled URL field creates the link; outside click with empty URL cancels; Escape cancels without creatingadded saving when changing text in the input
Summary by Sourcery
Rework the link creation and editing UI to use a unified compact two-row form and tooltip-style controls, and update link handling behavior on popup close and outside clicks.
New Features:
Bug Fixes:
Enhancements: