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.

Sign up for GitHub

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

Jump to bottom

Python Node Editor Visual Update #13732

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Python Node Editor Visual Update #13732

merged 4 commits into from
Feb 24, 2023

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Feb 7, 2023 •
edited
Loading

Purpose

This PR ensures Python node's UI color and styling are set in such a way as to homogenize with new sidebar extension theming. For more information on the task, refer to the following Figma link.

This PR will be followed by further development and implementation of all the required changes and updates.

UI Before

before

UI After (with user interactions)

after

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

  • overall theme update
  • icons update
  • icons hover interaction
  • tooltip update
  • color scheme for highlight syntax update
  • Escape key now closes the editor
  • TODO: check for unsaved changes on 'esc'. Update: this is handled in a follow-up PR based on this one.

Reviewers

@sm6srw
@Amoursol

FYIs

@Jingyi-Wen

dnenov added 4 commits February 1, 2023 17:34
@dnenov
Python editor visual restyle - initial commit
0d48fd5 
- initial commit for python editor visual restyle
- for general idea of the desired scope, visit https://www.figma.com/file/1q7EWQGYO7pPDhyLf8nuwW/Python-node-editor-restyling?node-id=0%3A1&t=QGCSTKllX8Q7OnhK-0
@dnenov
Merge remote-tracking branch 'upstream/master' into dnenov-pythonnode…
dc5d25d 
…editor-visualupdate
@dnenov
Icon Update
b6bef8b 
- updated python scrip editor icons
@dnenov
Hover icon update
277f9d7 
- minor update for one of the icons
@Amoursol
Copy link
Contributor

@mjkkirschner - Are there any performance issues (I'm assuming so) of taking 32x32 pixel icons and scaling them down to 16 x 16 pixels in code? They do appear crisper than native 16 x 16 pixels, but we want to know at what cost, performance or otherwise. @dnenov is currently doing this process.

@dnenov
Copy link
Collaborator Author

dnenov commented Feb 21, 2023 •
edited
Loading

@mjkkirschner - Are there any performance issues (I'm assuming so) of taking 32x32 pixel icons and scaling them down to 16 x 16 pixels in code? They do appear crisper than native 16 x 16 pixels, but we want to know at what cost, performance or otherwise. @dnenov is currently doing this process.

I can confirm that this was an oversight on my part. The following PR contains the actual icons, which are in native 24x24px to be displayed in the same resolution. No up or down scaling will take place.

Actually, it looks like I wasn't building with the updated icons, so this issue is not resolved. Below is a test between two resources from Figma, one at 24x24px vs the other at 48x48px.

image

These are my screen settings.

image

@dnenov dnenov changed the title Dnenov Python Node Editor Visual Update Python Node Editor Visual Update Feb 21, 2023
@dnenov dnenov mentioned this pull request Feb 21, 2023
Merged
8 tasks
@sm6srw sm6srw marked this pull request as ready for review February 22, 2023 13:49
@sm6srw
Copy link
Contributor

sm6srw commented Feb 22, 2023

Any icon adjustments will be in the part 2 PR.

sm6srw
sm6srw approved these changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@sm6srw sm6srw left a comment

Choose a reason for hiding this comment

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

LGTM!

@dnenov
Copy link
Collaborator Author

dnenov commented Feb 24, 2023

@mjkkirschner - Are there any performance issues (I'm assuming so) of taking 32x32 pixel icons and scaling them down to 16 x 16 pixels in code? They do appear crisper than native 16 x 16 pixels, but we want to know at what cost, performance or otherwise. @dnenov is currently doing this process.

I can confirm that this was an oversight on my part. The following PR contains the actual icons, which are in native 24x24px to be displayed in the same resolution. No up or down scaling will take place.

Actually, it looks like I wasn't building with the updated icons, so this issue is not resolved. Below is a test between two resources from Figma, one at 24x24px vs the other at 48x48px.

It was decided to keep the 48x48, this is done in #13763

@sm6srw sm6srw merged commit 0b1a5cf into DynamoDS:master Feb 24, 2023
@chuongmep
Copy link
Contributor

chuongmep commented Feb 24, 2023 •
edited
Loading

@ team, any people have felt red color is better ? Can we have any option to change color ? It looks like my eyes are a bit poor at the moment

@QilongTang
Copy link
Contributor

@ team, any people have felt red color is better ? Can we have any option to change color ? It looks like my eyes are a bit poor at the moment

Do you mean red color for save and run?

@dnenov
Copy link
Collaborator Author

dnenov commented Feb 28, 2023

@ team, any people have felt red color is better ? Can we have any option to change color ? It looks like my eyes are a bit poor at the moment

Do you mean red color for save and run?

Just to help out the conversation, this is what we are currently working towards:

image

The save and run color repeats the Dynamo run color.

@chuongmep
Copy link
Contributor

@QilongTang no, I mean is color of code.

@dnenov dnenov mentioned this pull request Feb 28, 2023
Merged
8 tasks
@dnenov
Copy link
Collaborator Author

dnenov commented Feb 28, 2023

@QilongTang no, I mean is color of code.

Just as a reference, this is the final PR part of this design effort, which deals exclusively with the color scheme of the reserved words. On our side it is @Amoursol and @Jingyi-Wen that should be able to shed more light on the decision about colors.

@chuongmep
Copy link
Contributor

Thanks @dnenov for clearly information, I will look to this.

@Jingyi-Wen
Copy link

@QilongTang no, I mean is color of code.

Hi @chuongmep! Just to clarify - do you mean the keywords such as "import" and "from" should be red? The reason we are changing the color scheme is to have better contrast. The new pink-ish color does have a better contrast ratio, compared to the previous red-ish dark pink.

@chuongmep
Copy link
Contributor

Hi @Jingyi-Wen, I not ignore that color is not good, just about combine color problem inside code, but I think a pull request for part 2 is to do that : #13783, this also is color i using every day in rider IDE

sm6srw pushed a commit to sm6srw/Dynamo that referenced this pull request Mar 29, 2023
@dnenov @sm6srw
Python Node Editor Visual Update (DynamoDS#13732)
1f50536 
* Python editor visual restyle - initial commit

- initial commit for python editor visual restyle
- for general idea of the desired scope, visit https://www.figma.com/file/1q7EWQGYO7pPDhyLf8nuwW/Python-node-editor-restyling?node-id=0%3A1&t=QGCSTKllX8Q7OnhK-0

* Icon Update

- updated python scrip editor icons

* Hover icon update

- minor update for one of the icons
sm6srw pushed a commit that referenced this pull request Apr 5, 2023
@dnenov @sm6srw
Python Node Editor Visual Update (#13732)
e41b5fc 
* Python editor visual restyle - initial commit

- initial commit for python editor visual restyle
- for general idea of the desired scope, visit https://www.figma.com/file/1q7EWQGYO7pPDhyLf8nuwW/Python-node-editor-restyling?node-id=0%3A1&t=QGCSTKllX8Q7OnhK-0

* Icon Update

- updated python scrip editor icons

* Hover icon update

- minor update for one of the icons
This was referenced Aug 29, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sm6srw sm6srw sm6srw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dnenov @Amoursol @sm6srw @chuongmep @QilongTang @Jingyi-Wen