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

[23Q1T1] Fade-out Node Glyphs #12664

Merged
merged 12 commits into from
Mar 23, 2022
Merged

[23Q1T1] Fade-out Node Glyphs #12664

merged 12 commits into from
Mar 23, 2022

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Feb 28, 2022 •
edited by sm6srw
Loading

Purpose

The first step of implementing the changes to the Color Coding behavior of Dynamo Nodes. This PR adds fade-out behavior to the stack panel containing the visual-cue glyphs (preview/frozen).

Commit 4 - Glyphs Overlay Implementation (2 options for glyph position)

(preferred)

glyphs_over

glyphs_under

Commit 3
node_color_overlay
Commit 2
frozen_glyph_implementation
Commit 1
glyph_fade_out

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

Updates the visual-cue glyph behavior when zoomed-in/out.

Reviewers

@sm6srw

FYI

@Amoursol

@dnenov
[23Q1T1] Fade-out Node Glyphs
74e1508 
Fade-out behavior to GlyphStackPanel hosting the icon/frozen glyphs.
@Amoursol Amoursol requested a review from sm6srw March 1, 2022 21:03
dnenov added 2 commits March 4, 2022 17:34
@dnenov
frozen_glyph_implementation
b24384e 
- added new png icons to be used in Node State visualization
- implemented the frozen glyph as part of the glyph stack panel
- the frozen glyph will be triggered upon freezing the node
@dnenov
Node Color Overlay
50bb70b 
- Node Color Overlay border zoom fade in and out styles added to allow overlay to be displayed using the same ZoomToBooleanConverter value (0.4) as the other UI elements
- Added a nodeColorOverlayZoomOut border to kick in after the zoomed out value is achieved
- The previous only border renamed from nodeColorOverlay to nodeColorOverlayZoomIn
- This allows one border to display Frozen/PreviewOff states when Zoom > 0.4 and another border to display Frozen/PreviewOff/Warning/Error/Info when Zoom < 0.4
- Added a HandleColorOverlayChange method to handle both WarningBarColor and NodeOverlayColor triggers
- Added GetBorderColor() method to assign both the correct color overlay to the zoomed out border and the correct glyph to be display in preparation for the glyphs implementation
- Further added a lot of infrastructure to handle the glyph display
@sm6srw
Copy link
Contributor

sm6srw commented Mar 7, 2022

You have one failing test: DynamoCoreWpfTests.NodeViewCustomizationTests.WatchImageCoreContainsImage

@sm6srw
Copy link
Contributor

sm6srw commented Mar 7, 2022 •
edited
Loading

Looks good. Looks like you are close to also enabling the display of the glyphs? Do you want to include that in this PR or in a separate PR? Can you also look into adding a few tests?

@dnenov
Copy link
Collaborator Author

dnenov commented Mar 8, 2022

Thanks, Jorgen. Only the .xaml part left more or less, let's do it in the same PR.

I'd love to add tests, but I have two main problems in that regard - a conceptual and a knowledge-based one. I started reading through the existing tests and getting a holistic enough view of the code base to allow me to write a test looks .. a bit intimidating at the moment. On the conceptual level, I would appreciate your support on what should be a well-placed test? I think I only have 1 method so far I'd like tested, but I am not sure if I am right or wrong.

I will finish up the code and before I get on with the tests, I will maybe chase you up for a quick chat on Slack?

@sm6srw
Copy link
Contributor

sm6srw commented Mar 8, 2022

Great, sounds like a plan. Ping me when you want to chat.

dnenov added 5 commits March 8, 2022 23:04
@dnenov
Glyphs Overlay Implementation
3992af6 
- Added Glyphs to Zoomed-Out State
- Added 3 New Converters
- Opacity of the frozen Border now bound to the zoomed state to correctly account for the initial starting frame (the Style animation then handles the behavior over time)
- The Grid hosting the 3 Glyphs is a bit wonky, makes sure the Warning/Error/Info states are always the first Icon, even if there are 1, 2 or 3 visible icons
@dnenov
Frozen Glyph Update
a1bc63b 
- more prominent color for frozen glyph #8EDCFF
@dnenov
Frozen > Preview Off - Priority Changed
56c69f2 
- changed priority for frozen and preview off border. Now will correctly prioritize Frozen over Preview-Off when zoomed out
@dnenov
Test Fix
db6c542 
- images in NodeView named for ease of use
- WatchImageCoreContainsImage test amended to accommodate code changes to NodeView
@dnenov
Test Zoom Node Color States Added
43cedb0 
- added a VM test to check Node View Model state border colors for NoWarning, Hidden, Frozen, Warning, Error states
- added the ZoomNodeColorStates.dyn test file containing all possible states to test against
- TODO - add test case for Info once it's released
sm6srw
sm6srw reviewed Mar 11, 2022
View reviewed changes
Copy link
Contributor

@sm6srw sm6srw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great! Do you feel like we have enough test coverage or do you think we need some tests that checks the properties on the node model?

dnenov added 4 commits March 21, 2022 09:25
@dnenov
Zoom Glyph Grid Collapse
871e459 
- the Gird containing all zoom state icons will now collapse if zoom level is over 0.4 (zoomed-in).
- this should incur less performance increase (before glyphs were simply given 0 opacity).
- added a similar visibility collapse control to the color border signifying the different states (hidden, frozen, info, warning, error) - the border will now collapse if zoom level is over 0.4
@dnenov
Test Zoom Node Model Glyph Properties Added
6eb92ee 
- added a new View Model test to check if the Image Sources for the state glyphs are set correctly
@dnenov
Test Zoom Change Visibility
cd2e148 
- added a test to assert that the elements shown in the zoomed-out (less than 0.4) node state are collapsed, and visible in the zoomed-in (more than 0.4) state
@dnenov
Merge remote-tracking branch 'upstream/master' into node-color-coding
ee081ae
@sm6srw
Copy link
Contributor

sm6srw commented Mar 22, 2022

@dnenov There is one failing test but it is unrelated to your work. I have restarted the tests.

sm6srw
sm6srw approved these changes Mar 22, 2022
View reviewed changes
Copy link
Contributor

@sm6srw sm6srw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All tests passed this time (but the status of the PR is not updated for some reason).

@sm6srw sm6srw merged commit b348f27 into DynamoDS:master Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjkkirschner mjkkirschner mjkkirschner left review comments

@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.
3 participants
@dnenov @sm6srw @mjkkirschner