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: Adjust icon colors for increased contrast #15964

Merged
merged 14 commits into from
Jul 29, 2021

Conversation

isabela-pf
Copy link
Collaborator

@isabela-pf isabela-pf commented Jul 1, 2021

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)

Screen Shot 2021-07-01 at 3 46 05 PM

Screen Shot 2021-07-01 at 3 47 47 PM

Spyder 5's light and dark mode icons in the toolbars don't all meet WCAG 2.1 contrast standards for non-text elements. While WCAG is a web-focused set of guidelines, contrast is not a web-specific problem so this makes sense in desktop applications as well. This PR is another tiny step for a more accessible Spyder.

Changes made:

  • Replace definition of ICON_2 and ICON_3 in both light and dark modes.
  • Replace non-font icons that use ICON_2 and ICON_3 in both light and dark modes. These are all SVGs in the images directory.
  • Change the disabled button opacity for toolbars to 65% opacity. (I'm still looking for this.) This is no longer a part of this PR.

Issue(s) Resolved

Addresses part of ux-improvements #42. There may be work in this area in the future to see if we can come up with a better solution.

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:
isabela-pf

@isabela-pf
Copy link
Collaborator Author

/show binder

@github-actions
Copy link

github-actions bot commented Jul 1, 2021

Binder 👈 Launch a Binder instance on this branch

@isabela-pf
Copy link
Collaborator Author

I've gotten feedback that the disabled button opacity is inherited from QDarkStyleSheet right now, and that we can either override it within Spyder or put in a PR and request a release of QDarkStyleSheet.

Right now I'm leaning towards overriding it simply because it seems like a small change that is potentially only needed for Spyder. I'm open to feedback on this, though.

@ccordoba12 ccordoba12 added this to the v5.1.0 milestone Jul 6, 2021
@isabela-pf
Copy link
Collaborator Author

isabela-pf commented Jul 8, 2021

Meeting feedback

  • Update branch
  • Double check that changes still work
  • Make 65% opacity for disabled buttons a QDarkStyleSheet PR

@isabela-pf
Copy link
Collaborator Author

I talked with @steff456 because I couldn't find anything that looked to me like a disabled button state for the toolbar buttons in QDarkStyleSheet. This does need to happen in Spyder so I'll be adding it to this PR. Here's why:

  • She figured out that this is because QDarkStyleSheet's disabled button styling isn't using the opacity we use in Spyder.
  • Notice how the above link shows styling for QToolButton, a widget Spyder does import. The thing is that Spyder uses QToolButton for many button types and changes here might change more buttons that we expect.
  • I wanted to try these changes out, and this seems like the place where opacity is applied. I also thinks this is an automatically generated file and changes here will be overridden. I'm now looking for where this is generated from.

@pep8speaks
Copy link

pep8speaks commented Jul 23, 2021

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

Line 119:20: W292 no newline at end of file

Comment last updated at 2021-07-29 14:16:11 UTC

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 @isabela-pf for helping us to improve this! I left an initial review for you.

spyder/utils/palette.py Show resolved Hide resolved
spyder/utils/palette.py Outdated Show resolved Hide resolved
spyder/utils/palette.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

Hey @isabela-pf, this requires a new release of QtAwesome with support for setting alpha in icon colors. For now I opened PR spyder-ide/qtawesome#166 to add that functionality.

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.

@isabela-pf, I left one last comment for you then this should be ready.

spyder/utils/palette.py Outdated Show resolved Hide resolved
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.

@isabela-pf, I'm sorry but I missed another couple of details that were missing from your revert. I left suggestions for them below.

spyder/utils/icon_manager.py Outdated Show resolved Hide resolved
spyder/utils/icon_manager.py Outdated Show resolved Hide resolved
isabela-pf and others added 2 commits July 29, 2021 07:15
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
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 a lot @isabela-pf for your help with this!

@ccordoba12 ccordoba12 merged commit 22f932e into spyder-ide:5.x Jul 29, 2021
ccordoba12 added a commit that referenced this pull request Jul 29, 2021
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.

None yet

3 participants