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(helper): update vue 2 template and helper #270

Merged
merged 5 commits into from
Sep 14, 2023

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Sep 7, 2023

What?

This PR updates the vue 2 template. Vue 2 helper + template used to have provider / inject pattern, and now that we've removed it, there is no such thing as useFieldPlugin in the vue 2 helper and the template. So what if we just remove vue 2 helper at all? We just need a little bit of snippet in the vue 2 template, and we're not actively supporting vue 2 anyway.

Why?

JIRA: EXT-1958

How to test? (optional)

@vercel
Copy link

vercel bot commented Sep 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plugin-sandbox ❌ Failed (Inspect) Sep 14, 2023 3:04pm

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented Sep 7, 2023

@eunjae-lee eunjae-lee force-pushed the EXT-1919-helper-update-vue-3-helper branch from 12b72ba to 034633d Compare September 8, 2023 08:50
@eunjae-lee eunjae-lee force-pushed the EXT-1958-helper-update-vue-2-helper branch from 32790b5 to 3cfc73a Compare September 8, 2023 08:50
@eunjae-lee eunjae-lee force-pushed the EXT-1919-helper-update-vue-3-helper branch from 034633d to 492af27 Compare September 8, 2023 08:57
@eunjae-lee eunjae-lee force-pushed the EXT-1958-helper-update-vue-2-helper branch from 3cfc73a to 9fa6859 Compare September 8, 2023 08:57
@eunjae-lee eunjae-lee force-pushed the EXT-1919-helper-update-vue-3-helper branch from 492af27 to dc1cb16 Compare September 8, 2023 09:05
@eunjae-lee eunjae-lee force-pushed the EXT-1958-helper-update-vue-2-helper branch from 9fa6859 to 1c24fb3 Compare September 8, 2023 09:05
@eunjae-lee eunjae-lee force-pushed the EXT-1958-helper-update-vue-2-helper branch from 1c24fb3 to b9f08cc Compare September 8, 2023 09:26
@eunjae-lee eunjae-lee marked this pull request as ready for review September 11, 2023 09:57
@eunjae-lee eunjae-lee requested review from a team and demetriusfeijoo and removed request for a team September 11, 2023 09:59
@demetriusfeijoo
Copy link
Contributor

@eunjae-lee, I think removing all the vue2 helper would not be bad, IMO.

However, how should we handle the FieldPlugin file?

I think we could remove this file since it seems not to be used anyway. what do you think?

@eunjae-lee
Copy link
Contributor Author

@demetriusfeijoo if we get rid of vue 2 helper, then we should actually delete this folder, but I didn't yet because we haven't decided yet.

And actually that empty FieldPlugin.vue file is a placeholder for users. We provide an example inside the template, but users can delete the whole example and start from scratch with the FieldPlugin.vue file (which is bare minimum)

@demetriusfeijoo
Copy link
Contributor

And actually that empty FieldPlugin.vue file is a placeholder for users. We provide an example inside the template, but users can delete the whole example and start from scratch with the FieldPlugin.vue file (which is bare minimum)

Oh, I see @eunjae-lee.

I think once we stop using the FieldPluginProvider inside the template, we can remove also the helper, for sure.
This way we avoid some developers using it and blocking us from removing it easily in the future?!

I mean, I was checking here, and after removing the FieldPluginProvider we would have anything else to offer from the helper, right? As we're not going to support this version, maybe the best way would be to remove the helper, indeed.

I was taking a better look at how the new approach would look and I started asking myself on how to make the logic below reusable between both FieldExample/index.vue and FieldPlugin.vue files.

image

Do you have any idea here? As we don't have hooks here (for useFieldPlugin()), may we use Vue mixins?!
What do you think @eunjae-lee?

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented Sep 13, 2023

Do you have any idea here? As we don't have hooks here (for useFieldPlugin()), may we use Vue mixins?! What do you think @eunjae-lee?

@demetriusfeijoo hey, thanks for pointing this out. I forgot about this part. added 61be914

@demetriusfeijoo
Copy link
Contributor

Hey @eunjae-lee, thanks for the adjustment. 🙌

What about getting rid of the helper? Do you have a better position on that?

@@ -0,0 +1,11 @@
#!/usr/bin/env zx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also replaced copy-helpers.sh with copy-helpers.mjs which seems easier to maintain.

Copy link
Contributor

@demetriusfeijoo demetriusfeijoo left a comment

Choose a reason for hiding this comment

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

It looks good to me @eunjae-lee.

Thanks for you effort on this 🙌

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented Sep 14, 2023

Merge Activity

  • Sep 14, 10:27 AM: @eunjae-lee started a stack merge that includes this pull request via Graphite.
  • Sep 14, 10:28 AM: Graphite failed to merge this pull request due to internal failures. Try your merge again, or report a bug if you see this consistently.
  • Sep 14, 10:39 AM: @eunjae-lee started a stack merge that includes this pull request via Graphite.
  • Sep 14, 10:39 AM: Graphite couldn't merge this pull request because a downstack PR fix(helper): update vue3 helper #269 failed to merge.

Base automatically changed from EXT-1919-helper-update-vue-3-helper to main September 14, 2023 14:53
@eunjae-lee eunjae-lee force-pushed the EXT-1958-helper-update-vue-2-helper branch from d02944d to 111b0e0 Compare September 14, 2023 14:55
@eunjae-lee eunjae-lee merged commit 1dc6487 into main Sep 14, 2023
@eunjae-lee eunjae-lee deleted the EXT-1958-helper-update-vue-2-helper branch September 14, 2023 14:55
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.

2 participants