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: Start spinner in dependencies dialog only when it's visible and make some improvements to the dialog's UI #21616

Merged
merged 8 commits into from
Dec 16, 2023

Conversation

ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Dec 14, 2023

Description of Changes

  • Increase our minimal required version of QtAwesome to 1.3.0 to do that.
  • Make some UI improvements to that dialog:
    • Fix spinner color in the light theme.
    • Increase padding to 15px.
    • Add notes to a single html list.
    • Add a better title and remove icon because it doesn't look good in the task bar.
    • Set foreground color of optional/missing dependencies in light theme to white so it has good contrast.
    • Disable copy button while waiting for dependencies to be computed.
    • Change color of dependency_ok icon so that it stands out in relation to the text.

Visual changes

Before

imagen

After

imagen

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:

This is necessary because that version has new API to start/stop spin
animations.
- Add notes to a single list.
- Add better title and remove icon because it doesn't look good.
- Add more margins so the content looks better.
Also, set the same css for enabled and disabled buttons in dialogs.
That's necessary to correctly change the state of a button from disabled
to enabled.
@ccordoba12 ccordoba12 changed the title PR: Start spinner in dependencies dialog when it's visible PR: Start spinner in dependencies dialog only when it's visible Dec 14, 2023
@dalthviz dalthviz added this to the v6.0alpha3 milestone Dec 14, 2023
@ccordoba12 ccordoba12 force-pushed the start-stop-spinner-deps branch from a22c572 to e90b5a3 Compare December 14, 2023 21:17
@pep8speaks
Copy link

pep8speaks commented Dec 15, 2023

Hello @ccordoba12! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 355:80: E501 line too long (98 > 79 characters)

Comment last updated at 2023-12-15 18:30:18 UTC

@ccordoba12 ccordoba12 force-pushed the start-stop-spinner-deps branch from eada236 to 7c20b22 Compare December 15, 2023 18:12
Also, change color of dependency_ok icon to make stand out in relation
to the text.
@ccordoba12 ccordoba12 force-pushed the start-stop-spinner-deps branch from 7c20b22 to 5a69be4 Compare December 15, 2023 18:30
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @ccordoba12 ! I just left a comment pointing that maybe it could be worthy to create a follow-up issue for the Find in Files segfault that was happening and that is handled here by skipping the main_widget.py file from the modules_test script.

Other than that this LGTM 👍

Edit - Also, just in case, this is how the dialog is looking when waiting:

image

Comment on lines +118 to +120
if [[ $f == spyder/plugins/findinfiles/widgets/main_widget.py ]]; then
continue
fi
Copy link
Member

Choose a reason for hiding this comment

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

Should we created an issue to follow-up on this file run giving a segfault?

Copy link
Member Author

@ccordoba12 ccordoba12 Dec 16, 2023

Choose a reason for hiding this comment

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

I don't think that's necessary because I was unable to reproduce locally the segfault generated when running this file and it seems random in CIs.

@ccordoba12 ccordoba12 changed the title PR: Start spinner in dependencies dialog only when it's visible PR: Start spinner in dependencies dialog only when it's visible and make some improvements to its UI Dec 16, 2023
@ccordoba12 ccordoba12 merged commit 8149cca into spyder-ide:master Dec 16, 2023
22 checks passed
@ccordoba12 ccordoba12 deleted the start-stop-spinner-deps branch December 16, 2023 14:01
@ccordoba12 ccordoba12 changed the title PR: Start spinner in dependencies dialog only when it's visible and make some improvements to its UI PR: Start spinner in dependencies dialog only when it's visible and make some improvements to the dialog's UI Dec 18, 2023
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.

4 participants