Bug-Refactor-Type-determination-logic-in-turn-into-dropdown-component #3239
Conversation
🦋 Changeset detectedLatest commit: 6bcce76 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thanks! Could you update |
| if (!isDefined(commonSelection)) { | ||
| commonSelection = type; | ||
| } else if (commonSelection !== type) { | ||
| commonSelection = undefined; | ||
| } |
There was a problem hiding this comment.
Thanks for the PR! It looks like this logic might cause the commonSelection variable to flip between undefined and some value. For example, if the types we get are ['typeA', 'typeB', 'typeC', 'typeD', 'typeE'], the value of commonSelection after each step is ['typeA', undefined, 'typeC', undefined, 'typeE'].
There was a problem hiding this comment.
Thanks @12joan will improve this logic which will prevent flip with below logic
There was a problem hiding this comment.
This has the same problem.
type commonSelection uniqueTypeFound
------------------------------------------
N/A undefined false
typeA typeA true
typeB undefined false
typeC typeC true
typeD undefined false
typeE typeE true
Maybe get the type of the first node and then use nodes.every to check if they all match? I think that code would be easier to read than using mutable variables and nested conditionals.
| const codeBlockEntries = getNodeEntries(editor, { | ||
| match: (n) => isBlock(editor, n), | ||
| }); |
There was a problem hiding this comment.
Have you tested this for selections containing nested blocks, such as traditional (non-indent) lists?
There was a problem hiding this comment.
By non-indent lists you meant bulleted list and numbered list right if you are it is working on them
There was a problem hiding this comment.
Plate has two kinds of lists. Indent lists, like on the main playground, don't use nesting. Traditional lists, which are more accessible and follow the standard UL, OL and LI nesting structure, might pose an issue since your getNodeEntries query will return both the outer list element and all list item elements.
There was a problem hiding this comment.
I think passing mode: 'highest' to getNodeEntries might be a workable fix, but you would need to test this on https://platejs.org/docs/list. The 'highest' mode tells getNodeEntries not to descend into the children of any matching node.
There was a problem hiding this comment.
i tested and code is working on https://platejs.org/docs/list.
Hey @12joan thanks a lot for taking out time and PR review would keep you valuable comments in mind for my future pull requests
kumarajay0412
changed the title
@zbeyens I hope have followed the correct format for adding changeset |
| let commonSelection: string | undefined; | ||
| let uniqueTypeFound = false; | ||
| let firstNodeType: string = ELEMENT_PARAGRAPH; | ||
| let allNodesMatchFirstNode = false; | ||
| const codeBlockEntries = getNodeEntries(editor, { |
There was a problem hiding this comment.
All looks good now. The only other thing I can see is you might want to change this variable name.
|
@kumarajay0412 Looks good, thanks! Could you update |
…n-into-dropdown-componet
…n-into-dropdown-componet
The changes in
turn-into-dropdown-componentrefactor the logic used to determine the type of block in the editor. The new implementation imports additional functions (getNodeEntries, isBlock) and uses them to iterate over block nodes within the editor's selection. It then determines a common type or defaults to 'paragraph' if type differ among nodes. This approach replaces the previous method of finding a single node and checking its type, making the new logic more robust and capable of handling multiple nodes with potentially different types.I have discussed this topic in detail here