-
Notifications
You must be signed in to change notification settings - Fork 160
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
Correct topic count when node kind is not found in clipboardNodesMap #4690
Correct topic count when node kind is not found in clipboardNodesMap #4690
Conversation
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 tested this locally and it worked perfectly and moved things where I wanted them. I'm happy that this solution ended up being a bit less complicated than I thought when we were looking at it together 😄
I left one non-blocking code comment where I may be making incorrect assumptions to begin with so if that's the case feel free to ignore.
// Check contentNodesMap for node kind if missing in clipboardNodesMap | ||
if (!kind && node?.source_node_id) { | ||
const contentNode = contentNodesValues.find(n => n.node_id === node.source_node_id); | ||
kind = contentNode?.kind; |
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.
Non-blocking question: did you find that the ?.
was necessary here because kind
and/or source_node_id
was missing on some contents or was it to just avoid unhandled errors just in case?
I ask because if we should expect kind
on every contentNode
(which I think we should?) then using the ?.
might result in hard-to-debug issues when it isn't because it'll just count broken nodes as "resources" in any case here.
It's not crucial here, but in the if/else below it might be worth making the second condition like this (shortened for GH formatting).
if(kind === 'topic) { topicCount++ }
else if (kind) { resourceCount++ } // it's not undefined or null, so we know it's a non-topic resource
else { console.error("`kind` missing from content...") }
But, again, this only really matters if we should be able to expect kind
to always be defined in this context -- which may not be the case?
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.
Yes, we should be able to expect kind
on every contentNode
, the ?
is in place to avoid any potential errors, but you are right about potential difficulties with debugging so I will make this change.
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.
Requested change was made - code updates look good to me.
Summary
Description of the change(s) you made
This pull request modifies the computed property
topicAndResourceCount()
to address instances where items in theclipboardNodesMap
may be missing the nodekind
. It introduces the use ofcontentNodesMap
to retrieve thekind
when it is not present in theclipboardNodesMap
.Reviewer guidance
How can a reviewer test these changes?
References
Fixes #3744
Comments
Contributor's Checklist
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)