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

fix: [RTE] embed entries in correct place when using hotkeys #1400

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

MayaGillilan
Copy link
Contributor

Removing selection in #1354 and #1353 introduced a regression specifically for users of hotkey insertion. Focusing improvements in those PRs are retained.

@MayaGillilan MayaGillilan requested a review from a team as a code owner April 19, 2023 12:16
@YvesRijckaert
Copy link
Contributor

@MayaGillilanContentful Can we add a test that covers this please? So we don't remove it again in the future!

@MayaGillilan
Copy link
Contributor Author

MayaGillilan commented Apr 19, 2023

@MayaGillilanContentful Can we add a test that covers this please? So we don't remove it again in the future!

I thought about this, not sure if we can actually since we only have a mock slide-in on the field-editors testing which doesn't actually "exist" or trigger any of this sdk.navigator watching logic that Daniel implemented. Or do you have another idea on how we could test it?

If not, I can at least add a comment explaining why we need it

@chrishelgert
Copy link
Contributor

❓ Wouldn't a component/integration test work for that? 🤔

@MayaGillilan MayaGillilan enabled auto-merge (squash) April 20, 2023 08:41
@MayaGillilan MayaGillilan merged commit e9712b5 into master Apr 20, 2023
@MayaGillilan MayaGillilan deleted the TOL-1091 branch April 20, 2023 08:55
@YvesRijckaert
Copy link
Contributor

@MayaGillilanContentful if we don't add a test, can we add a comment explaining it above the code line? just making sure we know what it does in the future and we do not remove it again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants