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

PR: Show installer download percentage progress in the status bar (Application) #19793

Merged
merged 4 commits into from
Oct 15, 2022

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Oct 11, 2022

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

A preview:

installer_download

Issue(s) Resolved

Fixes #19112

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: dalthviz

@dalthviz dalthviz added this to the v5.4.0 milestone Oct 11, 2022
@dalthviz dalthviz self-assigned this Oct 11, 2022
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Small suggestion for you @dalthviz, otherwise looks good to me.

spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

@dalthviz, could you post a screenshot in the description above to see how this looks? Thanks!

Code style

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @dalthviz! This looks pretty cool!

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Looking pretty good, thanks! A couple small tweaks, and two other visual issues:

  • The horizontal spacing between the percentage number and the text in the status bar seems a little bit too much; could you reduce it by a 1/3 to 1/2?
  • Maybe out of scope, but the overall text seems vertically misaligned with the other status bar items and its own icon, looking lower by at least a few pixels. Maybe out of scope here, but any way to align it properly?

spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
@dalthviz
Copy link
Member Author

A preview with the latest changes:

imagen

Checking the alignment more in detail I think that the further difference in alignment comes from the interpreter status not having an icon. If I put an icon to it I got this:

imagen

Should I add the icon to the interpreter status bar @CAM-Gerlach @ccordoba12 ?

@CAM-Gerlach
Copy link
Member

A preview with the latest changes:

Thanks. The space between the percent number and the text to its left still seems a bit much; it could probably still be cut in half if that's possible

image

Should I add the icon to the interpreter status bar @CAM-Gerlach @ccordoba12 ?

👍 from me, it looks a lot better, though seems like best to do in a separate PR

@ccordoba12
Copy link
Member

Thanks. The space between the percent number and the text to its left still seems a bit much; it could probably still be cut in half if that's possible

This is probably a Qt layout/css issue, which is usually not easy to fix. I'm fine to merge this one as it is, unless @dalthviz is willing to invest more time to find a solution for it.

+1 from me, it looks a lot better, though seems like best to do in a separate PR

I agree. In addition, I think that shouldn't be a blocker for the 5.4.0 release.

@dalthviz
Copy link
Member Author

I have been checking and it could be related with the QLabel padding but it will need further research so sound goods to me to merge this as it is.

Regarding the icon for the interpreter status bar, should I create another PR for that, right?

@ccordoba12
Copy link
Member

Regarding the icon for the interpreter status bar, should I create another PR for that, right?

I'd like to discuss first what we should do about the two status widgets that show almost the same interpreter information.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM for now from a UX perspective, then, given those two follow ups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants