[#69706] show WP link input only for current user and remove on blur#105
[#69706] show WP link input only for current user and remove on blur#105ihordubas99 merged 6 commits intodevfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
akabiru
requested review from
a team and
judithroth
and removed request for
akabiru
judithroth
left a comment
judithroth
left a comment
There was a problem hiding this comment.
Removing on blur is working 👍
I also see that you tried to improve and clean up the code - thanks for that!
I have some remarks though that keep me from approving so far (see comments)
| border: none; | ||
| border-radius: var(--bn-border-radius-small); | ||
| margin: var(--spacer-s) 0; | ||
| padding: 0 var(--spacer-m); |
There was a problem hiding this comment.
I wonder why you changed margin and padding here? Did you want to change something else maybe?
Before it looked like this:
Now it looks like this:
There is also now an additional disabled scroll bar that should not be there.
There was a problem hiding this comment.
I’ve reverted the margin and padding changes those were added by mistake.
Regarding the scrollbar, I had added it intentionally at first because I thought it was a UI issue. It’s now gone as well. Thanks for catching that!
There was a problem hiding this comment.
Thanks for fixing the scrollbar! The additional margin is still there though. I tried to illustrate in the picture what I mean:
judithroth
left a comment
judithroth
left a comment
There was a problem hiding this comment.
Ok, I still found some things that should be changed, but overall this looks quite good so far 👍
| border: none; | ||
| border-radius: var(--bn-border-radius-small); | ||
| margin: var(--spacer-s) 0; | ||
| padding: 0 var(--spacer-m); |
There was a problem hiding this comment.
Thanks for fixing the scrollbar! The additional margin is still there though. I tried to illustrate in the picture what I mean:
| editor, | ||
| }: { | ||
| block: BlockProps; | ||
| editor: BlockNoteEditor<DefaultBlockSchema & { openProjectWorkPackage: ReturnType<typeof openprojectWorkPackageBlockConfig> }>; |
There was a problem hiding this comment.
Was it necessary to change the type of the editor? I remember BlockNote to sometimes be picky when not the correct type was used, although now it seems to work with <any>. I'm just wondering 😅
There was a problem hiding this comment.
I used <any> during development to avoid typing errors. Now I've changed everything back to the correct type, thanks!
| (editor: BlockNoteEditor<any>) => ({ | ||
| title: i18n.t("slashMenu.title"), | ||
| onItemClick: () => { | ||
| const insertedBlock = insertOrUpdateBlockForSlashMenu(editor, { |
There was a problem hiding this comment.
Would it be possible to wrap all this that should happen in onItemClick in a function?
So far openProjectWorkPackageSlashMenu was readable quite nice. Having all that code in here somehow makes it hard to see what the actual keys and values are for this.
There was a problem hiding this comment.
Thanks! I moved the click logic into a separate function handleOpenProjectWorkPackageClick to keep onItemClick clean and readable
| tabIndex={disableFocus ? -1 : 0} // tabIndex toggle could affect accessibility | ||
| style={disableFocus ? { pointerEvents: "none" } : undefined} | ||
| > | ||
| <div contentEditable={false}> |
There was a problem hiding this comment.
Is this the thing that prevents the deletion through an other user? If that's all that is needed it would be really nice 👍
judithroth
left a comment
judithroth
left a comment
There was a problem hiding this comment.
This looks good now 👍
2 participants
https://community.openproject.org/projects/communicator-stream/work_packages/69706/activity
Restrict WP link input visibility to the user creating the block by tracking active state, remove uninitialized blocks on input blur, reset dropdown and focus state, ensure keyboard navigation works correctly.