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

[2.x] Upgrade VueJS to version 3 #666

Merged
merged 14 commits into from
Feb 15, 2021
Merged

[2.x] Upgrade VueJS to version 3 #666

merged 14 commits into from
Feb 15, 2021

Conversation

octoper
Copy link
Contributor

@octoper octoper commented Feb 9, 2021

Upgrade Inertia stack from VueJS v2 to VueJS v3

@driesvints
Copy link
Member

@octoper fyi: this PR won't be reviewed until it's out of draft

@octoper
Copy link
Contributor Author

octoper commented Feb 9, 2021

Yeah I know that's why I marked it as daft 😅

@octoper octoper marked this pull request as ready for review February 10, 2021 15:00
@octoper
Copy link
Contributor Author

octoper commented Feb 10, 2021

cc @driesvints Ready for review!

@driesvints
Copy link
Member

@octoper can you please fix the StyleCI warnings so everything's green?

@octoper
Copy link
Contributor Author

octoper commented Feb 11, 2021

@driesvints Done!

@driesvints
Copy link
Member

@reinink does this look good to you?

this.$once('hook:destroyed', () => {
document.removeEventListener('keydown', closeOnEscape)
})

Copy link
Member

Choose a reason for hiding this comment

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

So where does this get removed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops totally forgot about that, I will add it tommorow, this hook removed at first because the depreciation of $once function
but this willbe moved in the beforeUnmount lifecycle hook without the $once function

@@ -1,5 +1,5 @@
<template>
<portal to="modal">
<Teleport to="#modal">
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested this, but do you really need the <div id="modal"></div>? I think you can just do to="body". Also, I'm pretty sure even Vue uses the lowercase version of <teleport> in their docs, so that might be more idiomatic.

Suggested change
<Teleport to="#modal">
<teleport to="body">

@@ -25,11 +25,13 @@
</transition>
</div>
</transition>
</portal>
</Teleport>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
</Teleport>
</teleport>

Comment on lines 22 to 24

<!-- Modal Teleport -->
<div id="modal"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required, as per comment above.

Suggested change
<!-- Modal Teleport -->
<div id="modal"></div>

@@ -30,7 +30,7 @@
<div class="mt-4" v-if="$page.props.jetstream.hasTermsAndPrivacyPolicyFeature">
<jet-label for="terms">
<div class="flex items-center">
<jet-checkbox name="terms" id="terms" v-model="form.terms" />
<jet-checkbox name="terms" id="terms" v-model:checked="form.terms"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space.

Suggested change
<jet-checkbox name="terms" id="terms" v-model:checked="form.terms"/>
<jet-checkbox name="terms" id="terms" v-model:checked="form.terms" />

@@ -269,16 +269,15 @@ protected function installInertiaStack()
$this->updateNodePackages(function ($packages) {
return [
'@inertiajs/inertia' => '^0.8.2',
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth bumping this to 0.8.4, given the recent axios vulnerability (see here).

Suggested change
'@inertiajs/inertia' => '^0.8.2',
'@inertiajs/inertia' => '^0.8.4',

@octoper
Copy link
Contributor Author

octoper commented Feb 12, 2021

Fixed everything @reinink and @taylorotwell mentioned

document.addEventListener('keydown', closeOnEscape)
},

unmounted() {
const closeOnEscape = (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, does redefining the entire function actually work when removing it? I don't think so. I think you need the reference to the original function to remove it.

Here's what I believe is the proper Vue 3 way to handle this. Remove both the mounted() and unmounted() methods, and then add a new setup() method instead. Something like this:

import { onMounted, onUnmounted } from 'vue'

setup () {
  const closeOnEscape = (e) => {
      if (this.open && e.keyCode === 27) {
          this.open = false
      }
  }
  
  onMounted(() => document.addEventListener('keydown', closeOnEscape))
  onUnmounted(() => document.removeEventListener('keydown', closeOnEscape))
},

Again, this is untested, so definitely do test it, but this should work. 👍

@@ -71,10 +73,6 @@
}

document.addEventListener('keydown', closeOnEscape)

this.$once('hook:destroyed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

However we solve this in the Dropdown.vue component, needs to also be done here.

@taylorotwell
Copy link
Member

@octoper is this PR ready to go from your perspective now?

@octoper
Copy link
Contributor Author

octoper commented Feb 13, 2021

Yeah it's ready from me @taylorotwell

@khatabwedaa
Copy link

when this will be merged?

@taylorotwell taylorotwell merged commit e06cf8e into laravel:2.x Feb 15, 2021
@taylorotwell
Copy link
Member

Thanks!

@AbdelElrafa
Copy link

@taylorotwell will you be tagging a release soon so that this can be used, or should I just target the branch?

@driesvints
Copy link
Member

@AbdelElrafa releases are always done on Tuesdays. You should only ever target a dev branch for development and not for production releases.

@octoper octoper deleted the vue3 branch February 15, 2021 17:47
@driesvints driesvints changed the title [3.x] Upgrade VueJS to version 3 [2.x] Upgrade VueJS to version 3 Feb 16, 2021
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.

6 participants