Skip to content

Conversation

@ZiyunShang
Copy link
Contributor

Purpose

Limit the tooltip width of Ports so that the bubble will not be very long.
LongToolTipBubble
ShortToolTipBubble

Declarations

Check these if you believe they are true

  • The code base 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.

Reviewers

@QilongTang @mjkkirschner

FYIs

@AndyDu1985

<dynui:DynamoToolTip AttachmentSide="{Binding Path=PortType, Converter={StaticResource PortToAttachmentConverter}}" Style="{DynamicResource ResourceKey=SLightToolTip}">
<Grid>
<TextBlock Text="{Binding Path=ToolTipContent}"></TextBlock>
<TextBlock MaxWidth="320" TextWrapping="Wrap" Text="{Binding Path=ToolTipContent}"></TextBlock>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate here how did you define 320? If possible can you move the number to be a const number property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that the node Rectangle ToolTip was set width as 320. (

<TextBlock MaxWidth="320"
)
So I just keep the same as it.

@QilongTang
Copy link
Contributor

I am fine with us fixing it this way for now, LGTM as long as you put TODO comment for our team to move those magic numbers to resource strings

@ZiyunShang
Copy link
Contributor Author

ZiyunShang commented Jun 25, 2019

OK @QilongTang @mjkkirschner , I have create an issue on DYN Jira(https://jira.autodesk.com/browse/DYN-2000), you can take a look at it if you have time ; )

@QilongTang
Copy link
Contributor

@ZiyunShang Would you provide Jira link here or @ me and @mjkkirschner in it, otherwise we will not get a notification? Thanks!

@ZiyunShang
Copy link
Contributor Author

@QilongTang Ok, I have add the link here.

@QilongTang QilongTang added the LGTM Looks good to me label Jun 25, 2019
@ZiyunShang
Copy link
Contributor Author

@QilongTang @mjkkirschner could you help merge this?

@QilongTang QilongTang merged commit 6049ec9 into DynamoDS:master Jun 25, 2019
mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM Looks good to me

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants