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 5530 library zoom scaling #13733

Merged
merged 15 commits into from
Feb 16, 2023

Conversation

filipeotero
Copy link
Contributor

Purpose

The purpose of this PR is to add a zoom scale for the Library in the prefence settings.

zoomScaleLibrary

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 @zeusongit

FYIs

@RobertGlobant20

get { return preferencesWindow; }
}

internal event Action OnPreferencesWindowChanged;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library extension didn't have access to the new PreferencesView object created. So, this event was added to be called after a new preferences window is opened.


void PreferencesWindowChanged()
{
this.dynamoView.PreferencesWindow.LibraryZoomScalingSlider.ValueChanged += DynamoSliderValueChanged;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a new PreferencesWindow is created, the value of the slider is subscribed.

@zeusongit
Copy link
Contributor

Can we have a pointer at 200% as well? I think it will look more natural that way, what do you think @Amoursol @Jingyi-Wen

@Amoursol
Copy link
Contributor

Amoursol commented Feb 9, 2023

Can we have a pointer at 200% as well? I think it will look more natural that way, what do you think @Amoursol @Jingyi-Wen

Yes please - a consistent spread across would be best.

@filipeotero
Copy link
Contributor Author

Added 200% pointer
image

@@ -8,6 +8,7 @@
<ShowCodeBlockLineNumber>false</ShowCodeBlockLineNumber>
<ShowConnector>false</ShowConnector>
<ShowConnectorToolTip>false</ShowConnectorToolTip>
<LibraryZoomScale>2.0361751152073739</LibraryZoomScale>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the value intended to be so long? Can we limit the digit length of the value serialized 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.

We can use float instead of double

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me

@@ -42,6 +44,8 @@ public class LibraryViewController : IDisposable
// TODO remove this when we can control the library state from Dynamo more precisely.
private bool disableObserver = false;

private static readonly string LibrarSlider = "notificationsButton";
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used?

this.dynamoView.PreferencesWindow.LibraryZoomScalingSlider.ValueChanged -= DynamoSliderValueChanged;
this.dynamoView.OnPreferencesWindowChanged -= PreferencesWindowChanged;

var dynamoViewWindow = dynamoWindow as DynamoView;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this line here?

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.

LGTM with a couple of comments, would you address those?

@filipeotero
Copy link
Contributor Author

I've also added localization for the expander header and adjusted the position of the percentage labels depending on the slider value.

@QilongTang
Copy link
Contributor

@RobertGlobant20 With your latest changes, the test WorkspaceContextMenu_TestIfInCanvasSearchHidesOnOpeningContextMenu still fails sporadically.

@QilongTang QilongTang merged commit 295e342 into DynamoDS:master Feb 16, 2023
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.

4 participants