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: Status bar switches to icon view too early #7576

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

VikalpRusia
Copy link
Contributor

Bug

Status bar icon collapses even when bar has space

Why?

Hard coded for giving space to the progress bar

More details

#6514

Changes

Screenshots

Screenshot 2024-10-04 at 12 45 57 AM Screenshot 2024-10-04 at 12 46 04 AM Screenshot 2024-10-04 at 12 46 11 AM Screenshot 2024-10-04 at 12 50 48 AM

closes #6514

@VikalpRusia VikalpRusia changed the title Fixes - https://github.com/rancher-sandbox/rancher-desktop/issues/6514 Fixes: Status bar switches to icon view too early Oct 3, 2024
@VikalpRusia VikalpRusia force-pushed the status-bar-fix branch 2 times, most recently from 742e4db to ba1ef59 Compare October 3, 2024 19:30
@VikalpRusia VikalpRusia changed the title Fixes: Status bar switches to icon view too early Fix: Status bar switches to icon view too early Oct 3, 2024
@gunamata gunamata requested a review from mook-as October 4, 2024 16:49
@jandubois
Copy link
Member

Thank you! I haven't looked at the PR, but I've tested it for a bit and have 2 comments:

  1. While the progress bar and status message are being displayed, the app still switches to the icon view pretty early. In the following screenshot, if I narrow the window just a few more pixels, it will switch to icon view:

CleanShot 2024-10-04 at 13 19 43@2x

I assume this happens because we reserve a certain amount of space for the status messages, even when they are actually much shorter?

This is not really important though; when the progress bar is gone the status line springs back to full width.

  1. The switch to icon view, when there is no status message and progress bar, happens slightly late. In the following screenshot you can see that most of the "moby" string is being cut off.

CleanShot 2024-10-04 at 13 20 55@2x

Note how the version string in my example is much longer than in yours. I don't know if you dynamically determine the width of the status bar, or just reserve a certain amount of space for it.

@jandubois
Copy link
Member

BTW, if you take a screenshot on macOS with ⌘⇧4, you can press the space bar while hovering over a window to just capture that window and don't have to select the rectangle manually (and include parts of the background).

Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

FWIW, I think we should not switch based on if the status bar is visible, because that means a lot of movement when the status bar pops up. But with a lack of proper UI people right now I defer to Jan for those decisions.

image
This does look like way too much whitespace though; note that the version isn't even visible here…

@@ -74,7 +74,7 @@ export default Vue.extend({

methods: {
submit() {
this.$emit('click', { action: this.action, image: this.image });
this.$emit('click', { action: this.action, image: this.image.trim() });
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this meant to go into a different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, its better if we create another PR was a backlog item.

@@ -61,20 +65,23 @@ export default Vue.extend({
<img
v-if="isSvgIcon"
class="item-icon icon-svg"
:class="{'make-icon-inline': isProgressBarVisible}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The various make-* classes are awkward:

  • The make- prefix is repetative, and doesn't really say much
  • They're all doing the same thing, but with lots of different names

I'd suggest just collapsed instead. Or, even better, move it to a parent element (the actual status bar?) and use CSS rules to select them.

@jandubois
Copy link
Member

@VikalpRusia Ping! Do you still intend to work on this?

For the mean time I'm going to turn it into draft mode, so it doesn't show up every day during our standup when we go through open PRs.

@jandubois jandubois marked this pull request as draft November 27, 2024 22:00
@VikalpRusia
Copy link
Contributor Author

@jandubois I will try to complete it this week. Thanks for making it draft

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.

Status bar switches to icon view too early
3 participants