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

[INT] Refactored some of the plugins #10943

Merged
merged 3 commits into from
May 6, 2024

Conversation

eva-vashkevich
Copy link
Member

@eva-vashkevich eva-vashkevich commented May 3, 2024

Summary

Fixes #10774
Refactors:

  • shell/plugins/resize.js
  • shell/plugins/v-select.js
  • shell/plugins/tooltip.js
  • shell/plugins/shortkey.js
  • shell/plugins/portal.js - is not used and has been removed
  • shell/plugins/portal-vue.js
  • shell/plugins/global-formatters.js

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

The entire application is affected since these plugins are globally used.
In particular:

  • Make sure that window can be resized and behavior matches old behavior
  • Make sure that select dropdown works and looks as expected
  • Make sure if you hover over elements tooltips are displayed as expected
  • Make sure shortkeys work ( ie using alt on side menu)
  • Make sure portal under advanced section in machine pools works
  • Make sure all components under @shell/components/formatter work as expected

Areas which could experience regressions

The entire application is affected since these plugins are globally used.

Screenshot/Video

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

Copy link
Member

@rak-phillip rak-phillip 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 splitting these PRs. I think this change looks good, there's just one area that we'll want to consider updating.

Comment on lines 22 to 24
console.warn('The implicit addition of global formatters has been deprecated in Rancher Shell and will be removed in a future version. Make sure to invoke `Vue.use(globalFormatters)` to maintain compatibility.');

Vue.use(globalFormatters);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we might only require these lines when running tests. Can we update this to use the same pattern as what we use in the i18n plugin now?

https://github.com/rancher/dashboard/pull/10923/files#diff-4964064b3ea3d66459e9eb386e2612aa8aea93c2d858eca54128348b498dcabb

@@ -1,10 +1,24 @@
import Vue from 'vue';
/* eslint-disable no-console */
import Vue from 'vue';const components = require.context('@shell/components/formatter', false, /[A-Z]\w+\.(vue)$/);
Copy link
Member

Choose a reason for hiding this comment

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

Did eslint incorrectly munge this line?

rak-phillip
rak-phillip previously approved these changes May 3, 2024
Copy link
Member

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

I just have one more comment about the content of a comment.

shell/plugins/global-formatters.js Outdated Show resolved Hide resolved
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
@eva-vashkevich eva-vashkevich merged commit 9fffaff into rancher:master May 6, 2024
26 checks passed
@eva-vashkevich eva-vashkevich added the QA/manual-test Indicates issue requires manually testing label May 6, 2024
@eva-vashkevich eva-vashkevich deleted the iss10774-p01 branch May 6, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extensions QA/manual-test Indicates issue requires manually testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Plugins and Directives to Provide a Path for Vue3
2 participants