Skip to content
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

[JS] Updated deprecated usage of keycode to code (#5837) #5997

Merged
merged 4 commits into from Jun 18, 2021
Merged

[JS] Updated deprecated usage of keycode to code (#5837) #5997

merged 4 commits into from Jun 18, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jun 11, 2021

Related Issue

Fixes #5837

Description

It has been identified that at various locations the deprecated keycode property was in use. This has been updated to use the code property instead.

How Verified

  1. Built locally
  2. Tested key commands on image element with select action
  3. Tested key commands on media element
Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Jun 11, 2021

Hi @ChristoperHowell. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

@ghost
Copy link

ghost commented Jun 11, 2021

CLA assistant check
All CLA requirements met.

@ghost ghost closed this Jun 11, 2021
@ghost ghost reopened this Jun 11, 2021
@paulcam206
Copy link
Member

Thanks for the PR, @ChristoperHowell 👍

Taking a look now :)

Copy link
Member

@paulcam206 paulcam206 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good -- @dclaux?

Copy link
Member

@dclaux dclaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but there are more uses of code in the designer code base that should also be updated (technically there are quite a few more in the adaptivecards-controls package as well but it's less important to update at this time)

@ghost
Copy link
Author

ghost commented Jun 12, 2021

Hi, @dclaux. I originally searched for keycode using VS code. This is what came back and I have fixed them. Would you be able to point me in the right direction to find more occurrences?

@ghost ghost removed the Needs: Author Feedback label Jun 12, 2021
@dclaux
Copy link
Member

dclaux commented Jun 14, 2021

@ChristoperHowell the other references are in the designer code base, which lives in the source\nodejs\adaptivecards-designer folder.

@ghost
Copy link
Author

ghost commented Jun 15, 2021

Thanks for the reply @dclaux. I’ve manually checked every file in the designer project and can find no other occurrences of the deprecated key code property.

From my understanding, the original issue requested that all occurrences of the deprecated key code property be updated to use the code property, which I have done this. Was there an additional requirement, or did I make a mistake in my understanding of this request? I can find other event properties being used such as the event shift key property. Would you like these modified to use the event code property?

Thanks for the input!

@dclaux
Copy link
Member

dclaux commented Jun 15, 2021

@ChristoperHowell my apologies. I do not know what I typed in the search box, but I must have typed something else than "keyCode", as I'm now not finding the references I mentioned before. Or maybe I just dreamt them.

Either way, your changes look good, so I'll go ahead and approve your PR then merge.

Thank you very much for this contribution!

@dclaux dclaux self-requested a review June 15, 2021 18:45
@ghost
Copy link
Author

ghost commented Jun 16, 2021

@dclaux, not a problem! Thanks for letting me contribute!

@dclaux dclaux enabled auto-merge (squash) June 18, 2021 19:47
@dclaux dclaux merged commit a09a401 into microsoft:main Jun 18, 2021
michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
…crosoft#5997)

Co-authored-by: David Claux <dclaux@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[JS] Replace keyCode with code everywhere
3 participants