-
Notifications
You must be signed in to change notification settings - Fork 302
Window Manager enhancements #15464
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
Window Manager enhancements #15464
Conversation
57e26e2 to
6705638
Compare
00ef10e to
43968c5
Compare
| @@ -0,0 +1,291 @@ | |||
| <script lang="ts" setup> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core changes are here. It adds the new side window
shell/config/store.js
Outdated
| '../store/type-map.js', | ||
| '../store/uiplugins.ts', | ||
| '../store/wm.js', | ||
| '../store/wm/index.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core changes are here. It adds the new wm/secondary store.
shell/utils/dynamic-importer.js
Outdated
|
|
||
| export function resolveWindowComponent(key) { | ||
| return require.resolve(`@shell/components/nav/WindowManager/${ key }`); | ||
| return require.resolve(`@shell/components/Console/${ key }`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decoupling Kubectl components from Primary window
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest using Window rather than Console - this is the auto-import of Vue components that can be shown in the window manager, I don't think its specifically console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
rak-phillip
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left another round of comments. One thing to note with regards to accessibility - we will need to ensure that focus is set to an element within the secondary panel when it is opened.
| // This should work at tab level, not global | ||
| // if (!!component.value || componentName.value === name) { | ||
| // _warn(`component is already loaded`, name, extensionId); | ||
| // return; | ||
| // } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove commented code
|
I think we could do this with less change. My thoughts on what we should do:
Not sure about renaming 'wm' to 'console' - I think the term 'console' is perhaps to specific. |
| this.$dispatch('wm/open', { | ||
| id: `kubectl-${ this.id }`, | ||
| label: this.$rootGetters['i18n/t']('wm.kubectlShell.title', { name: this.nameDisplay }), | ||
| label: this.$rootGetters['i18n/t']('console.kubectlShell.title', { name: this.nameDisplay }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping wm is fine - rather than console.
| @dragend="$refs.draggableZone.onDragEnd($event)" | ||
| > | ||
| <WindowManager @draggable="draggable=$event" /> | ||
| <PrimarySideWindow @draggable="draggable=$event" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest naming HorizontalWindowManager
| <WindowManager @draggable="draggable=$event" /> | ||
| <PrimarySideWindow @draggable="draggable=$event" /> | ||
| </div> | ||
| <SecondarySideWindow /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest naming VerticalWindowManager
|
Also, I'd be okay with disabling the window dragging for now (if that helps keep this simpler) and bringing it back when we re-factor to a cleaner single window manager. |
cd44c23 to
3b9ee62
Compare
- Decouple PrimarySideWindow from Console components - code refactoring - use Composition APIs - Extensions API - handle async enabling actions - Improve PrimarySideWindow resize - Add grab cursor to primary window Signed-off-by: Francesco Torchia <[email protected]>
34dc648 to
3e33d8e
Compare
|
@nwmac @rak-phillip I decided to re-implement the WindowManager functionality to fully support multiple windows and tabs dragging. |
60b7150 to
ac9f77c
Compare
ac9f77c to
7d17f89
Compare
rak-phillip
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a distinct change in behavior now. The previous implementation would allow dragging all tabs together to pin the entire group - this allowed for inactive tabs to be a drag target.
The new implementation allows individual tabs to be pinned to different areas, but only the active tab can be dragged.
pkg/rancher-components/src/components/RcDropdown/RcDropdownItemSelect.vue
Show resolved
Hide resolved
| } | ||
| height = Math.min(height, 3 * windowHeight / 4); | ||
|
|
||
| setDimensions({ height }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getters should be side-effect free. Both getters for width and height will commit a mutation. Perhaps, it would be better to initialize dimensions using the mounted hook or some other mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is mostly legacy code and it needs to be cleaned up.
However, the logic there is a bit complex, so I would leave this kind of refactoring for later to avoid the risk of breaking the set-dimensions behavior, if you are fine with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's create a new issue so we can follow up and address in a separate PR. This will also help to manage the scope of this change as well.
Signed-off-by: Francesco Torchia <[email protected]>
rak-phillip
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - The dragging functionality looks good and I think it's an overall improvement, especially with regards to flexibility.
Summary
Fixes #15505
Fixes #15527
Technical notes summary
It adds the multiple side windows and tabs dragging support.
Areas or cases that should be tested
Areas which could experience regressions
Screenshot/Video
Screencast.from.2025-10-06.09-58-04.webm
Checklist