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

DYN-6440 Custom Selection Overlapped #14960

Merged
merged 6 commits into from
Feb 26, 2024
Merged

DYN-6440 Custom Selection Overlapped #14960

merged 6 commits into from
Feb 26, 2024

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

Fixing problem with large text in the Custom Selection nodes.
When choosing a large string text from the ComboBox was overlapping other controls at left side then for fixing this bug I've added a new TextBlock that will hidden for all the nodes but will be shown just for Custom Selection nodes.
This new TextBlock will show "..." at the end of the string when trying to display a very large text.

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
  • This PR contains no files larger than 50 MB

Release Notes

Fixing problem with large text in the Custom Selection nodes.

Reviewers

@QilongTang

FYIs

@avidit

When choosing a large string text from the ComboBox was overlapping other controls at left side so for fixing this bug I've set the MaxWidth property and also I've added a tooltip showing the text (this will be useful when we have a large string).
Adding a new TextBlock that when has a large text will wrap the text show at the end the chars: "..."
@RobertGlobant20
Copy link
Contributor Author

This is a GIF showing the expected behavior for Custom Selection node.
RKRgSr8716

@QilongTang QilongTang added this to the 3.1 milestone Feb 22, 2024
Adding property in the Model so we can control the TextBlock Visibility.
Copy link

github-actions bot commented Feb 23, 2024

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0


formControl.BaseComboBox = dropdown;
formControl.BaseComboBox.SelectionChanged += BaseComboBox_SelectionChanged;
Copy link
Contributor

Choose a reason for hiding this comment

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

unsubscribe somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For unsubscribing the method I added the Dispose() method but not sure if this class is being disposed correctly or not.
It's supposed that if you derive from IDispose and implement the Dispose() method the right way to dispose the object should be by using the "using(instance)" or by calling object.Dispose() directly.
commit: c08ab0e

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks would you test when workspace is closed, if the dispose function is called? Just curious about the conclusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang you were right, when closing the workspace the Dispose method is being reached.
image

Added more comments for clarifying the use of the IsVisibleDropDownTextBlock  property.
Also I've added the Dispose() method for unsubscribing event handlers (although I'm not so sure if we are disposing this class correctly).
@QilongTang QilongTang merged commit f1fd7eb into DynamoDS:master Feb 26, 2024
21 of 22 checks passed
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.

2 participants