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

Make side panel handles size to content #11057

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

mmisol
Copy link
Collaborator

@mmisol mmisol commented Aug 26, 2020

Purpose

The handles for size panels were using a fixed size, which was not
enough to fit some translated texts. The handles are now allowed to
size to content by doing some extra gimnastics to rotate them in the
right way.

Here is a screenshot of how it looks with the German translation:
extension_handle

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

@DynamoDS/dynamo

The handles for size panels were using a fixed size, which was not
enough to fit some translated texts. The handles are now allowed to
size to content by doing some extra gimnastics to rotate them in the
right way.
@mmisol mmisol requested review from QilongTang and zeusongit August 26, 2020 15:40
@@ -1625,17 +1618,29 @@
Margin="5,5,5,5"
RenderTransformOrigin="0.5, 0.5">
<Image.RenderTransform>
<RotateTransform Angle="90" />
<RotateTransform Angle="-90" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what this is for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's for rotating a UI element. This is related to the gymnastics I mentioned. Since the rotation of the panel changed directions, this one needs to change as well.

@aparajit-pratap
Copy link
Contributor

Does the library handle look better as well with German characters?

@mmisol
Copy link
Collaborator Author

mmisol commented Aug 26, 2020

@aparajit-pratap Do you mean German characters not in English? I did not try, as the German translation did not have any for this case. Is there any particular text you would like to try? I can just write it literally in the control to try it

@aparajit-pratap
Copy link
Contributor

@aparajit-pratap Do you mean German characters not in English? I did not try, as the German translation did not have any for this case. Is there any particular text you would like to try? I can just write it literally in the control to try it

Yes, I meant any language as long as it is a long string, just so we can manually check that the library handle resizes as expected.

@mmisol
Copy link
Collaborator Author

mmisol commented Aug 26, 2020

Here is a sample showing a very long text. It eventually overlaps some of the canvas controls, but the handle adapts just fine. In any case, this is an hypothetical case, we don't have such long translations.

long_extension_text

@QilongTang
Copy link
Contributor

@mmisol Thanks taking a look now

@QilongTang
Copy link
Contributor

@mmisol Can you check the left side panel for me? Do we have similar issue there?
image

@@ -1516,7 +1516,7 @@
VerticalAlignment="Top"
Margin="0,310,0,0">
<Grid Background="#353535"
Width="100"
Width="Auto"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this is the only changed needed to fix the left side?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. No further gymnastics required on that side.

@mmisol
Copy link
Collaborator Author

mmisol commented Aug 26, 2020

Here is a longer example for the library side @QilongTang

long_library_text

FontWeight="Normal"
Margin="5,0,0,5"
Text="{x:Static p:Resources.ExtensionsViewTitle}">
</TextBlock>
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why did we only move this textblock for the right side but keep the change for left?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without getting into much detail, the main difference is that the centre of the rotation is different for each side, which in turn requires to do things differently in order to show the handles as we expect them.

}
}

private void ExtensionHandle_MouseEnter(object sender, MouseEventArgs e)
{
Grid g = (Grid)sender;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check the type of sender here is Grid before doing all of the following?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part did not really change. I would say it is fine like this given it is an event handler, and as such should not be reused.

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 just a minor comment

@mmisol mmisol merged commit 7e7196d into DynamoDS:master Aug 27, 2020
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.

3 participants