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

[Sc 65261] Migrate to Vue 3 #108

Merged
merged 7 commits into from
Feb 5, 2024

Conversation

ericyd
Copy link
Member

@ericyd ericyd commented Feb 1, 2024

Description of the change

  1. Update dependencies to Vue 3 (and corresponding versions of Vuex and vue-router)
  2. Update app creation, store creation, and router configuration
  3. Add the ui-components library (this was the whole reason to update to Vue 3)

Companion PRs:

Testing

This integration is fully functional in this state, you can clone and npm link it and test it locally. There are currently no functional changes, the only work of this PR is to update it to Vue 3.

Future work

  • The actual work described in the ticket, of auto-configuring the Page Scan stuff with a multi-select input

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change
  • Technical Debt
  • Documentation

Related tickets

https://app.shortcut.com/active-prospect/story/65261/trustedform-v4-insights-add-on-ui-should-prompt-for-required-and-forbidden-scan-text

Checklists

Development and Testing

  • Lint rules pass locally.
  • The code changed/added as part of this pull request has been covered with tests, or this PR does not alter production code.
  • All tests related to the changed code pass in development, or tests are not applicable.

Code Review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached.
  • At least two engineers have been added as "Reviewers" on the pull request.
  • Changes have been reviewed by at least two other engineers who did not write the code.
  • This branch has been rebased off master to be current.

Tracking

  • Issue from Shortcut/Jira has a link to this pull request.
  • This PR has a link to the issue in Shortcut.

QA

  • This branch has been deployed to staging and tested.

Copy link

@@ -19,7 +19,7 @@
<Navigation :onConfirm="onConfirm"/>
</div>
<div v-else-if="!isDataService">
<LoadingScreen :module-name="'TrustedForm ' + moduleName"/>
<LoadingScreen :onFinish="() => {/* NOOP */}" :module-name="'TrustedForm ' + moduleName"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

This avoids a new error from Vuex 4 which complains when a requested action doesn't exist. The error was being thrown from here and I have no idea why this code exists so I didn't feel comfortable removing it

@@ -1,20 +1,16 @@
import ui from 'leadconduit-integration-ui';
import Vue from 'vue';
import { createApp } from 'vue';
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not how lc-client instantiates the app, even though it is Vue 3. The way lc-client gets around this is with the @vue/compat lib, which treats things as if they were still Vue 2. Since integrations are such simple micro-apps, I think it makes more sense to "do it right" from the start and use the newer Vue 3 APIs.

Copy link

Choose a reason for hiding this comment

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

Thank you

@ericyd ericyd marked this pull request as ready for review February 1, 2024 22:12
@ericyd
Copy link
Member Author

ericyd commented Feb 1, 2024

@samullen and @johnrb2 I'm sorry but in a way, you volunteered yourselves for this PR review, by volunteering to review my other Vue 3 PRs 😅

Copy link

@samullen samullen left a comment

Choose a reason for hiding this comment

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

Thanks for doing things the "right" way

@@ -1,20 +1,16 @@
import ui from 'leadconduit-integration-ui';
import Vue from 'vue';
import { createApp } from 'vue';
Copy link

Choose a reason for hiding this comment

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

Thank you

@ericyd ericyd merged commit 8eccb42 into master Feb 5, 2024
3 checks passed
@ericyd ericyd deleted the sc-65261/trustedform-v4-insights-add-on-ui-should branch February 5, 2024 21:36
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.

None yet

3 participants