-
Notifications
You must be signed in to change notification settings - Fork 221
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
Updated the skillmap query parameter #6786
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
@@ -687,7 +687,7 @@ | |||
"ocvFrameUrl": "https://admin-ignite.microsoft.com" | |||
}, | |||
"queryVariants": { | |||
"skillsMap=1": { |
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 we know if this could break partners who might embed us? (For example, not sure if code ninjas uses skill maps).
My guess is probably not, but we should make sure.
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.
Ah, good point, @kiki-lee, do you know if Code Ninjas or anyone else uses this flag?
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.
Or @Jaqster?
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.
They don't use our tutorials or skillmaps, so I assume it's fine. Also, they're pinned to an older version of Arcade. I can't think of anyone else who serves us in a version that's not the offline app. I'm sure @abchatra would know better.
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 don't believe embedding skillmap works today. This should be ok
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.
Also we don't have to fix this. This is not use facing string.
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.
If it should be fine, I'm not sure I see the problem in updating it if we want the correct spelling of skillmap to persist throughout the code base.
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.
Seems fine, but if you wanted to be extra safe you could duplicate "skillsMap" into a new variant "skillmap".
Happened due to updating everything to the correct spelling of skillmap.
Related to microsoft/pxt#10421