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

feat: version compatible with vue 3 #95

Merged
merged 17 commits into from
Oct 16, 2020
Merged

Conversation

elevatebart
Copy link
Contributor

Hello @koca ,

Here is the first draft of the prism editor working with vue 3.
I have been asked to startup a vue-live engine for vue 3.

Would you be open to delivering this one on the @next channel?

Do you need anything more?

@koca
Copy link
Owner

koca commented Oct 15, 2020

Hey @elevatebart! Thank you for the PR ♥️ i will review this after work(today).

@elevatebart
Copy link
Contributor Author

I just found out that the @testing-library/vue was indirectly dependent on vue 2 (through their version of the vue test utils).
It means that the unit tests will not work until they deliver their upgrade.

Could we to deliver this version as an alpha?

@koca
Copy link
Owner

koca commented Oct 16, 2020

Why do i see my old commits? There should be only 1 commit i think 🤔

@elevatebart
Copy link
Contributor Author

Yep @koca I forgot to mention that.

Note that my PR is based on the next branch.

The next branch is behind master by those 20 commits.
I started by rebasing master into next before starting the refactor.

The only change that matters here is the last commit:

394b1ce

@koca
Copy link
Owner

koca commented Oct 16, 2020

can you rebase this branch and update the PR?

@koca
Copy link
Owner

koca commented Oct 16, 2020

oh i see its not from master branch

immediate: true,
handler(newVal: string): void {
handler(this: any, newVal: string): void {
Copy link
Owner

Choose a reason for hiding this comment

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

why did you add this:any to some methods?

@@ -104,31 +105,31 @@ export const PrismEditor = Vue.extend({
},
content: {
immediate: true,
handler(): void {
handler(this: any): void {
Copy link
Owner

Choose a reason for hiding this comment

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

same this:any

Comment on lines 4 to 15
const KEYCODE_ENTER = 13;
const KEYCODE_TAB = 9;
const KEYCODE_BACKSPACE = 8;
const KEYCODE_Y = 89;
const KEYCODE_Z = 90;
const KEYCODE_M = 77;
const KEYCODE_PARENS = 57;
const KEYCODE_BRACKETS = 219;
const KEYCODE_QUOTE = 222;
const KEYCODE_BACK_QUOTE = 192;
const KEYCODE_ESCAPE = 27;
const KEYCODE_ENTER = 'Enter';
const KEYCODE_TAB = 'Tab';
const KEYCODE_BACKSPACE = 'Backspace';
const KEYCODE_Y = 'y';
const KEYCODE_Z = 'z';
const KEYCODE_M = 'm';
const KEYCODE_PARENS = '(';
const KEYCODE_BRACKETS = '{';
const KEYCODE_QUOTE = '"';
const KEYCODE_BACK_QUOTE = '`';
const KEYCODE_ESCAPE = 'Escape';
Copy link
Owner

Choose a reason for hiding this comment

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

these are not keycodes anymore what was the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

event.keyCode is deprecated
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode

and event.key seems compatible with IE9+
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

It has nothing to do with vue 3 and I should remove this change from this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

i think its okay we can keep the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2020-10-16 at 2 37 44 PM

@elevatebart
Copy link
Contributor Author

Thank you @koca for the comments,

this: any turned out to be unnecessary so I removed it.

the keyCode property is deprecated but should not prevent vue.3 from working so I removed this change too.

I hope it simplifies the review a little.

Sorry again for the confusion.

@@ -55,7 +56,7 @@ export const PrismEditor = Vue.extend({
type: Boolean,
default: false,
},
value: {
modelValue: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -261,7 +262,7 @@ export const PrismEditor = Vue.extend({
input.selectionStart = record.selectionStart;
input.selectionEnd = record.selectionEnd;

this.$emit('input', record.value);
this.$emit('update:modelValue', record.value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

// onKeyDown(e);
this.$emit('keydown', e);
// onKeyDown(e);
this.$emit('keydown', e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.$listeners has been removed in vue 3 (or is null as far as I can tell) which triggers an undefined error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After investigation, this.$listeners exists only if it has been implemented, with an @click= or an @keydown=
It should be protected against null.
Since it is deprecated anyway, I removed it completely.

Copy link
Owner

Choose a reason for hiding this comment

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

yep they've merged $listeners to $attrs. So $attrs has both $attrs and $listeners in vue 3

@@ -512,27 +511,23 @@ export const PrismEditor = Vue.extend({
}
},
},
render(h): VNode {
render() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

h is now imported from vue

@elevatebart
Copy link
Contributor Author

I have added comments in this commit to explain my changes

77bb9af

@koca
Copy link
Owner

koca commented Oct 16, 2020

There is only console.log left can you please remove it and i've tested on my local, seems to be working fine. We can't run tests but I'll publish the alpha version for you. Thank you @elevatebart 😊👍

@koca koca merged commit c5b9f9f into koca:feature/next Oct 16, 2020
koca added a commit that referenced this pull request Oct 16, 2020
Co-authored-by: Mesut Koca <[email protected]>
Co-authored-by: Lane Wagner <[email protected]>
Co-authored-by: Jan Östlund <[email protected]>
@koca
Copy link
Owner

koca commented Oct 16, 2020

Hey @elevatebart just released v2.0.0-alpha.1

example vue 3 csb: https://codesandbox.io/s/vue-3-support-for-vue-prism-editor-fm7v5

install:

npm install vue-prism-editor@alpha

or

yarn add vue-prism-editor@alpha

cheers 🎉

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.

4 participants