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

Tweaks to new cluster switch feature #10707

Merged

Conversation

aalves08
Copy link
Member

@aalves08 aalves08 commented Mar 27, 2024

Summary

Fixes #10706

  • add missing translation to hardcoded string and adjust of copy
  • change key shortcut to ALT key only
  • remove lodash usage
  • adjust e2e test

Occurred changes and/or fixed issues

Note: Pressing COMMAND + F5 opens a system-like panel in mac OS, which might lead to the "icon" getting stuck and visible. Pressing ALT multiple times seems to get it unstuck. We cannot prevent system key combinations from possible overrides and it's side-effects, imo. If there's additional info that might change this, let me know.

video.mov

Technical notes summary

Areas or cases that should be tested

  • Bring up a second cluster in ready state (meaning you can navigate to cluster explorer)
  • In that cluster, go to project/namespaces
  • press the key alt
  • while pressing the key, make sure the icons appear next to the cluster boxes in the main nav, and click on the local cluster
  • assert that the route loaded is the project/namespaces but in the local cluster

Areas which could experience regressions

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

…ge key shortcut to ALT key only + remove lodash usage
@aalves08 aalves08 added this to the v2.9.0 milestone Mar 27, 2024
@aalves08 aalves08 self-assigned this Mar 27, 2024
contentText = 'Switch clusters and keeps location';
} else if (this.routeCombo &&
typeof item === 'object' &&
!Array.isArray(item) &&
Copy link
Member

Choose a reason for hiding this comment

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

why wouldn't array be a valid type here? it looks like lodash recognized them as objects https://lodash.com/docs/4.17.15#isObject

Copy link
Member Author

@aalves08 aalves08 Apr 1, 2024

Choose a reason for hiding this comment

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

It can be either a string or an object and logic changes based on that (different applications of the same function). Just making sure we aren't running code for a different var type if someone misuses this function.

Ah 😅 , I was missusing the isObject lodash function here! I though it ruled out array's... I understand why they allow it, but it it's kinda misleading in the sense that they are usually helper functions for stuff just like this 😛

@eva-vashkevich
Copy link
Member

I've tried switching between clusters and it works as expected. I did notice the "stuck" problem but I am also unsure how we can work around it.

@aalves08
Copy link
Member Author

aalves08 commented Apr 1, 2024

I've tried switching between clusters and it works as expected. I did notice the "stuck" problem but I am also unsure how we can work around it.

Me neither... It's "system" stuff. You'll eventually find something system related that will leave you in a weird state.

Copy link
Member

@eva-vashkevich eva-vashkevich left a comment

Choose a reason for hiding this comment

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

lgtm

@aalves08 aalves08 merged commit bf435d6 into rancher:master Apr 1, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tweaks to new cluster switch feature
2 participants