-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: page editor #240
feat: page editor #240
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
||
const containerRef = useRef<HTMLDivElement>(null); | ||
useEffect(() => setSelectedIndex(0), [items]); | ||
useImperativeHandle(ref, () => ({ |
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.
We should probably add this keyboard navigation to the TypeDialog in a future PR
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.
Why are we using useIMperativeHandle
instead of a useEffect
?
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.
It feels a little better to hook into ProseMirror's handleKeyDown event and call it in the command-extension.tsx file rather than having a global document listener in the react component, but both could work!
entityStore.updateEditorBlocks(editor); | ||
}, | ||
}, | ||
[editable] |
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.
useEditor takes a dependency array - I think there's potential for problems if entityStore.editorJson isn't fully hydrated by the time we reach this point.
We don't always want editorJson as a dependency however, since it will update onBlur and defocus and close our command popover triggered by the "/" or the "+" button.
Will know if this is a problem once we put it into our actual entity page.
<EditorContent editor={editor} /> | ||
<FloatingMenu editor={editor}> | ||
<div className="absolute -left-12 -top-3"> | ||
<SquareButton onClick={() => editor.chain().focus().insertContent('/').run()} icon="plus" /> |
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.
A bit of a hack to open up the command menu, but dealing with the internals of Prosemirror can be very annoying.
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.
Can you turn this into a function called openCommandMenu
or something so it's clear what this chain of calls does?
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.
Yee
); | ||
}); | ||
|
||
export const TableNodeChildren = React.memo(function TableNodeComponent({ |
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.
Was running into a lot of excessive rerendering of the table, went ham on memoization to address the issue
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.
Do you know why it was re-rendering? I'd rather not memoize everything if we know the root cause and if performance isn't actually an issue.
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.
Good point. The problem is with const { types } = useEntityTable();
- if we remove the useMemo and memoized TableNodeChildren component, the table will flicker on every blur. It looks like we can safely remove the other memoizations.
@@ -45,9 +44,12 @@ export default function EntityPage(props: Props) { | |||
return ( | |||
<EntityStoreProvider | |||
id={props.id} | |||
name={props.name} |
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.
Name and the blockIdsTriple and blockTriples are now attributes of EntityStoreProvider
Perhaps name should be computed off triples, would that be cleaner @baiirun
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 probably okay, but now there's places in the codebase where we read the entity name using triples and places where we don't.
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'll switch it to a computed for cleanliness sake
suggestion: { | ||
items: ({ query }) => { | ||
return commandItems | ||
.filter(v => v.command) |
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.
What is this filter doing? Don't all our commands in command-items
have a command?
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.
After the command menu opens, we can filter the menu options by typing in it! I left a comment, but we can also just remove this feature...
popup = tippy('body', { | ||
getReferenceClientRect: props.clientRect as any, // fixme | ||
appendTo: () => document.body, | ||
content: reactRenderer.element, | ||
showOnCreate: true, | ||
interactive: true, | ||
trigger: 'manual', | ||
placement: 'bottom-start', | ||
}); | ||
}, |
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.
Can we somehow use Radix's popover elements instead of bringing in a new dependency?
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 attempted to follow the code used in ueberdosis/tiptap#2305 for a more Reacty way of doing things, but Radix doesn't seem to expose a way to position a popover w/o a Popover.Trigger element.
const [selectedIndex, setSelectedIndex] = useState(0); | ||
const [mode, setMode] = useState<CommandListMode>('select-block'); | ||
|
||
const tableItem = items.find(item => item.title === 'Table') as CommandSuggestionItem; |
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.
Could this ever return undefined? When casting like this we gotta be careful that we aren't accidentally hiding a bug.
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.
Cool, I extracted tableCommandItem to make sure we don't need this awkward find statement
}; | ||
|
||
const invokeItem = (item: CommandSuggestionItem) => { | ||
if (!item) { |
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.
When would we not pass an item? Based on the type parameter I'd expect it to always be non-null/undefined.
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.
Good point!
|
||
const containerRef = useRef<HTMLDivElement>(null); | ||
useEffect(() => setSelectedIndex(0), [items]); | ||
useImperativeHandle(ref, () => ({ |
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.
Why are we using useIMperativeHandle
instead of a useEffect
?
|
||
const selectedType = useMemo(() => { | ||
return types.find(type => type.entityId === typeId) as Triple; | ||
}, [types.length, typeId]); // eslint-disable-line react-hooks/exhaustive-deps |
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.
Are we sure this won't cause stale data if we get two types
arrays that are the same length? We could stringify the types
array if we know it's never very large.
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.
JSON.stringify works!
); | ||
}); | ||
|
||
export const TableNodeChildren = React.memo(function TableNodeComponent({ |
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.
Do you know why it was re-rendering? I'd rather not memoize everything if we know the root cause and if performance isn't actually an issue.
@@ -45,9 +44,12 @@ export default function EntityPage(props: Props) { | |||
return ( | |||
<EntityStoreProvider | |||
id={props.id} | |||
name={props.name} |
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 probably okay, but now there's places in the codebase where we read the entity name using triples and places where we don't.
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'm making changes to the main [entityId].tsx
page which will probably cause issues when you try and migrate this v2 page over. We might want to get in my versioning changes on the page before we migrate the editor over.
initialRows?: Row[]; | ||
initialSelectedType?: TripleType | null; | ||
initialTypes?: TripleType[]; | ||
initialColumns?: Column[]; |
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.
Why are we making these optional?
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.
Whoops, that should not have been committed
No description provided.