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

SCLD-17783 Removes usage of deprecated draftjs props #3

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

robhoff-laerdal
Copy link
Collaborator

@robhoff-laerdal robhoff-laerdal commented Oct 8, 2024

SCLD-17783

Changes

  • onTab, onUpArrow, and onDownArrow are all deprecated functions and cause very noisy console warnings in SCLD
  • react-draft-wysiwyg exposes these props on the Editor component, although we do not use them.
  • Despite not using them, I tried to maintain the same functionality while removing the deprecated usages. I followed the draftjs warning suggestion and added a custom keyBindingFn to the draftjs Editor. This function just checks for the 3 special cases, calls those functions, and then returns the default value for that key.
  • It is worth noting that getDefaultKeyBinding does not have a value for tab, up, or down, so the behavior between the custom props and the default behavior are totally separate.

Testing

  • I will do local testing to verify the fix and to verify the package still works

@@ -58,6 +58,7 @@
"dependencies": {
"classnames": "^2.2.6",
"draftjs-utils": "^0.10.2",
"fbjs": "^1.0.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the package that draft-js uses to get Keys

@robhoff-laerdal robhoff-laerdal requested review from a team, matthewbordas and elliot-murdock and removed request for a team October 8, 2024 15:11
@elliot-murdock elliot-murdock added the ready-to-merge All code reviews and QA checks are completed label Oct 11, 2024
@robhoff-laerdal
Copy link
Collaborator Author

I did some testing locally with this and it seems that we need to build the contents of dist in order for this change to be reflected. I had some issues with that so further investigation will be required.

@elliot-murdock elliot-murdock removed the ready-to-merge All code reviews and QA checks are completed label Oct 15, 2024
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.

3 participants