-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Improve versions of debug icons, add new cell icon and optimize svg icons #20874
Conversation
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.
Thanks @conradolandia for your work on this!
And congratulations for your first PR in Spyder!!
spyder/images/dark/new_cell.svg
Outdated
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'm not sure if we should use magenta as the color for this icon. Instead, I think we could simply show it in white for the dark theme and in black for the light one.
What do you think?
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.
Yeah, at least IMO the magenta color is a little more distracting to me and I'm not sure I understand the motivation given that cells aren't shown as magenta by default.
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.
Good point too: the current magenta draws too much attention to that icon unnecessarily.
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.
Agreed. Was the original color of the previous icon and I didn't think about changing it, but you guys are right.
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.
The changes generally look good to me (I've long advocated for swapping the pause and play positions), though I do agree with @ccordoba12 's comments with regard to the color of the cell icon—see my comment there.
…o improving-icons Forgot to pull before pushing, my bad.
I've changed the |
@conradolandia, I reviewed your PR locally and it needs some minor additional changes to work as expected:
In addition, I noticed that the new but that's a quite simple fix. You also said:
Great work @conradolandia! I checked them at high resolution and I see no difference with the previous icons. |
…o improving-icons Pull before pushing new commit
Ok, it should be fixed. By the way... I got a different idea for the This new version is more consistent with the rest of the icon system, regarding what we visually mean as a "new cell". I created it with no color bits for now, but we can discuss that. Maybe some color on the What do you think? |
Yeah, that's a great idea and a lot more visually consistent to me with both the appearance and symbology of the other icons. Agreed 👍 |
I totally agree with @CAM-Gerlach. That icon looks quite nice in our toolbar along the others, so great work @conradolandia! Please upload it instead of the current cell icon so we can merge this PR. |
Done! |
…o improving-icons Merged remote with local to update remote changes
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.
Thanks @conradolandia for your work on this!
Note: There's a test slot that started to hang but it's unrelated to the changes here, so I'm going to merge it.
Description of Changes
debug
,debug_cell
,debug_line
) both in Dark and Light versions.new_cell
.I've added two replacements for icons
debug_cell
anddebug_selection
, plus a new genericdebug
icon. Another icon was added fornew_cell
.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:
Andres Montoya