-
Notifications
You must be signed in to change notification settings - Fork 7
Refactor to use prosemirror-adapter-react for attribute menu #1147
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.
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
Conversation
view.current?.dispatch(tr); | ||
} | ||
|
||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding this fixes a flushSync
error. i don't really know why, and avoiding that kind of nonsense does seem like a strong pro of using the other view library
panelPosition: PanelProps; | ||
viewRef: React.RefObject<EditorView | null>; | ||
} | ||
export function AttributePanel() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i made smaller changes to this component than were probably necessary:
- got rid of its local state and used the parent's (maybe a mistake?). ideally it wouldn't depend on the
node
in the parent component state being up to date at all and could instead use prosemirror as the source of truth (perhaps just from the selection, or maybe directly in the plugin state). it should probably also submit all of its values at once instead of using these onchange handlers on individual inputs. - moved it inline in the doc (also maybe a mistake)
- conditionally render the special link menu (not really functional, i wouldn't worry about it)
- updated some styles/moved a few to tailwind classes
view.dispatch( | ||
view.state.tr.setSelection( | ||
new TextSelection( | ||
view.state.doc.resolve(pos) | ||
// view.state.doc.resolve(getPos() + node.nodeSize) | ||
) | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was from an attempt to make the attribute panel work based on the selection, rather than manually setting the node
in the panelProps. it's not really working at the moment so you'll probably want to remove this
buttonText = `${node.type.name}${node.type.name === "heading" ? ` ${node.attrs.level}` : ""}`; | ||
} | ||
|
||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where I would make the buttons for lists/any nested blocks more readable. Perhaps we could group them by depth? Or make them show on some kind of inline hover when they're not at the top level of the document. The way we handle this in legacy for suggested edits may be helpful to look at, if that's something you're tackling here.
}, | ||
props: { | ||
decorations(state) { | ||
// return this.getState(state)?.decorations; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncommenting this should use our state logic to only rerender decorations in parts of the doc that change instead of crawling the whole doc each time, as recommended by marijn. But even without that, memoization makes this fast enough to work for now.
stepMap.forEach((oldStart, oldEnd, newStart, newEnd) => { | ||
mappedDecorations = mappedDecorations.remove(mappedDecorations.find(newStart, newEnd)); | ||
editorState.doc.nodesBetween(newStart, newEnd, (node, pos, parent, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this, so if you do use it, I'd make sure this is using the right combo of old/new positions and the respective editorstate vs previouseditorstate. Also there's a chance these are just the totally wrong functions to use 😓
// handleClick: (view, pos, event) => { | ||
// const $pos = view.state.doc.resolve(pos); | ||
|
||
// const mark = $pos.marks()[0]; | ||
|
||
// const range = getMarkRange($pos, mark.type); | ||
|
||
// const node = $pos.parent; | ||
|
||
// const isLink = Boolean( | ||
// baseSchema.marks.link.isInSet([ | ||
// ...(view.state.storedMarks || []), | ||
// ...($pos.marks() || []), | ||
// ]) | ||
// ); | ||
// console.log({ pos, node, event }); | ||
// setPanelPosition({ | ||
// ...panelPosition, | ||
// isOpen: !panelPosition.isOpen, | ||
// isLink, | ||
// pos: range?.from || pos, | ||
// node, | ||
// }); | ||
|
||
// return true; | ||
// }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using this in an attempt to make clicking into any mark open its menu, while also allowing the cursor to stay inside the editor, rather than having to click the tiny button and unfocusing the editor. But since the attribute panel depends on the node including the entire text in the mark and the pos pointing to the beginning of the mark, this needs some tweaking.
Co-authored-by: Kalil Smith-Nuevelle <[email protected]>
* Add id and class to link attributes * Add link menu based on #1147 Co-authored-by: Kalil Smith-Nuevelle <[email protected]> * Make attribute panel open/close a bit more intuitively * Make link menu update attributes * Add form errors * Add advanced options * Attribute panel opening and closing tests * Add some link tests * Add state to blank story * Fix locators * Fix type * Add failing test * Fix some browser differences * Only autofocus when there is no url * Change the link form to submit on blur so that we always have valid data in the pm doc * Clean up * Clean up some more * Make scroll work kinda * Make panel appear on right properly * Fix browser clicking discrepencies * Update selector for hard breaks test * await expect 🤦 * useRef for container and restore `relative` * Remove extra prop * Add exception in test for webkit CI * Properly pass ref. should make tests more stable --------- Co-authored-by: Kalil Smith-Nuevelle <[email protected]>
Not for merging, just putting this here in case it's useful for @allisonking