-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fix(preference-tree-widget): sync. Theia and VSCode behavior about extensions' node naming #12929
Conversation
2af2e8d
to
f0692f9
Compare
…tensions' node naming Contributed by STMicroelectronics Signed-off-by: Vincent GRENET <[email protected]>
f0692f9
to
07cb37a
Compare
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.
Alright, behavior looks mostly good, I just have a few remarks on the code and one on the behavior: The TypeScript extension provides preferences starting with typescript.*
and javascript.*
. Since both use the same title
, but with a different group ID, these get listed twice as separate groups in the preferences:
VSCode shows them as expected only as a single group.
} | ||
|
||
export namespace CompositeTreeNode { | ||
export const is = (node: BaseTreeNode): node is CompositeTreeNode => 'depth' in node && 'title' in node; |
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.
Suggestion: Since title
is an optional property, using 'title' in node
can lead to false negative results.
export const is = (node: BaseTreeNode): node is CompositeTreeNode => 'depth' in node && 'title' in node; | |
export const is = (node: BaseTreeNode): node is CompositeTreeNode => 'depth' in node; |
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 to me. I'll integrate this suggestion.
const keys = Object.keys(config.properties); | ||
keys.forEach(key => { config.properties[key].title = config.title; }); |
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.
Suggestion: We don't need to make it so complicated by first getting all the keys.
const keys = Object.keys(config.properties); | |
keys.forEach(key => { config.properties[key].title = config.title; }); | |
Object.values(config.properties).forEach(property => property.title = config.title); |
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 to me. I'll integrate this suggestion.
Ok got it. Good point. I'll work a bit more on tree generator to get proper "merge" of such nodes sticking VSCode rendering. |
@VGRSTM Do you plan to update this PR? |
Have got no time to allocate to up to now |
Superseded by #13819. Closing this PR. |
What it does
Fix #12928
Contributed by STMicroelectronics
Signed-off-by: Vincent GRENET [email protected]
How to test
Follow-ups
Review checklist
Reminder for reviewers