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 4843 watch node smaller #12829

Merged
merged 25 commits into from
May 2, 2022
Merged

Dyn 4843 watch node smaller #12829

merged 25 commits into from
May 2, 2022

Conversation

jesusalvino
Copy link
Contributor

@jesusalvino jesusalvino commented Apr 26, 2022

Purpose

  • Set the minimum height to the Watch Node when is created.
  • Set the proper heights to the Watch Nodes when a Dynamo file is opened: The minimum height when is a single line, otherwise the Default height.

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

###Reviewers
@QilongTang

###FYIs
@RobertGlobant20 @filipeotero

@jesusalvino
Copy link
Contributor Author

WatchSize

@QilongTang QilongTang requested a review from reddyashish April 26, 2022 02:07
@QilongTang
Copy link
Contributor

FYI: @Amoursol @reddyashish Notes from standup, for single line of data, we should try to hide vertical scrollbar if possible

@QilongTang QilongTang added this to the 2.15.0 milestone Apr 26, 2022
@jesusalvino
Copy link
Contributor Author

WatchSize_2
Updating the added ACs

@@ -26,15 +28,42 @@ public WatchTree()
void WatchTree_Loaded(object sender, RoutedEventArgs e)
{
_vm = this.DataContext as WatchViewModel;
_vm.PropertyChanged += _vm_PropertyChanged;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we unsubscribe this somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

Looks good on my side. @Amoursol how is the new look?

@Amoursol
Copy link
Contributor

Looks good on my side. @Amoursol how is the new look?

Looking much better for sure! Is it possible to also not have it quite as wide if we are returning a smaller digit like a 2?

Also, @jesusalvino can you please make a quick GIF of also showcasing the following:

small = 2; long = 10000; list = 0..20; doubleList = [0..20,20..40]; longSingle = Line.ByStartPointEndPoint( Point.ByCoordinates(10,20,30), Point.ByCoordinates(40,50,60));

WatchNodeTesting.mov

This will allow us to test a range of cases, and see how the node behaves.

@jesusalvino
Copy link
Contributor Author

jesusalvino commented Apr 30, 2022

Looks good on my side. @Amoursol how is the new look?

Looking much better for sure! Is it possible to also not have it quite as wide if we are returning a smaller digit like a 2?

Also, @jesusalvino can you please make a quick GIF of also showcasing the following:

small = 2; long = 10000; list = 0..20; doubleList = [0..20,20..40]; longSingle = Line.ByStartPointEndPoint( Point.ByCoordinates(10,20,30), Point.ByCoordinates(40,50,60));
WatchNodeTesting.mov

This will allow us to test a range of cases, and see how the node behaves.
WatchSize5

Done @Amoursol , notice the default width is 200, it turns dynamic according to the source

@reddyashish
Copy link
Contributor

reddyashish commented May 2, 2022

@jesusalvino @Amoursol @QilongTang What do you think about setting a maximum width and having a horizontal scroll bar. The last case seems pretty wide.

Copy link
Contributor

@reddyashish reddyashish left a comment

Choose a reason for hiding this comment

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

Looks good to me once we confirm the max width case.

@QilongTang
Copy link
Contributor

Please confirm @Amoursol I also think the last case is too wide to display

@Amoursol
Copy link
Contributor

Amoursol commented May 2, 2022

@jesusalvino @Amoursol @QilongTang What do you think about setting a maximum width and having a horizontal scroll bar. The last case seems pretty wide.

Agreed, it sounded good on paper but looks a bit wild in practice! Can we please go with a width that is reasonable, then the user can expand again?

Worth testing = 2x the width of the watch node in 2.14 as a max stretch default.

@jesusalvino
Copy link
Contributor Author

jesusalvino commented May 2, 2022

WatchSize6

@Amoursol The max width of a Watch node will be its default 2X = 400, if the line exceeds it, the horizontal scrollbar will be available for the user

@Amoursol
Copy link
Contributor

Amoursol commented May 2, 2022

WatchSize6

@Amoursol The max width of a Watch node will be its default 2X = 400, if the line exceeds it, the horizontal scrollbar will be available for the user

This is looking AWESOME! Thank you @jesusalvino. Did you manage to have it not as wide when returning a small number such as 2 alone?

@jesusalvino
Copy link
Contributor Author

WatchSize6
@Amoursol The max width of a Watch node will be its default 2X = 400, if the line exceeds it, the horizontal scrollbar will be available for the user

This is looking AWESOME! Thank you @jesusalvino. Did you manage to have it not as wide when returning a small number such as 2 alone?

imagen

yes

@Amoursol
Copy link
Contributor

Amoursol commented May 2, 2022

WatchSize6
@Amoursol The max width of a Watch node will be its default 2X = 400, if the line exceeds it, the horizontal scrollbar will be available for the user

This is looking AWESOME! Thank you @jesusalvino. Did you manage to have it not as wide when returning a small number such as 2 alone?

imagen

yes

Looks perfect to me - thank you! Consider it done from my side 😊

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.

5 participants