Refresh Dep Viewer by QilongTang · Pull Request #10046 · DynamoDS/Dynamo

QilongTang · 2019-10-10T01:49:18Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Adding a button for user to refresh dep viewer manually.

Tooltip:

HoverOverState:

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

@DynamoDS/dynamo

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

mjkkirschner · 2019-10-10T02:03:35Z

src/DynamoCoreWpf/Views/Core/NodeView.xaml

             xmlns:dynui="clr-namespace:Dynamo.UI.Controls"
             xmlns:p="clr-namespace:Dynamo.Wpf.Properties"
             xmlns:configuration="clr-namespace:Dynamo.Configuration;assembly=DynamoCore"
-             xmlns:fa="http://schemas.fontawesome.io/icons/"


might this negatively effect nodeViews which reference font awesome?

Shouldn't be the case, at least this name space is not referenced at all.. I can also remove if there are known issues but I can't think of any side effect

mjkkirschner · 2019-10-10T15:13:35Z

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml

+                                         Icon="Refresh" 
+                                         Foreground="{DynamicResource MemberButtonText}" 
+                                         ToolTip="{x:Static w:Resources.RefreshButtonTooltipText}" 
+                                         MouseLeftButtonDown="Refresh_MouseLeftButtonDown"/>


I know we said we would not go too far UI wise - but at a minimum - I think this button should have a tooltip and hover interaction to make clear it is clickable and not just an icon.

@mjkkirschner Yes, please review the tooltip text, it is included in this PR

thanks, any thoughts on the hover interaction? like a highlight?

mjkkirschner · 2019-10-10T15:13:49Z

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml

                <!-- Package Version -->
                <DataGridTextColumn 
-                    Header="Version" 
+                    Header="{x:Static w:Resources.VersionHeaderText}" 


Thanks, this should fix the localization bug

mjkkirschner · 2019-10-10T15:14:14Z

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

+
+        private void Refresh_MouseLeftButtonDown(object sender, System.Windows.Input.MouseButtonEventArgs e)
+        {
+            DependencyRegen(currentWorkspace);


does this function have testing?

Needs UI automation test for this one.

I actually meant for the DependencyRegen function - I think if that is covered this is likely good enough without a new test.

@mjkkirschner Yes, we had a few unit tests covering that from the past

mjkkirschner · 2019-10-10T15:14:29Z

src/WorkspaceDependencyViewExtension/WorkspaceDependencyViewExtension.csproj

  <ItemGroup>
+    <Reference Include="FontAwesome.WPF, Version=4.3.0.35981, Culture=neutral, processorArchitecture=MSIL">
+      <SpecificVersion>False</SpecificVersion>
+      <HintPath>..\..\extern\FontAwesome\FontAwesome.WPF.dll</HintPath>


we dont use nuget for this?

@mjkkirschner We don't. we are referencing an old version of FA (4.3.0) and keeping a hard copy of it in our repo. Although the latest WPF version on nuget is 4.7.0, not so different. We should update when FA 5.0 for WPF available and switch to use nuget referece because it will provide much more.

mjkkirschner · 2019-10-10T15:14:47Z

looks good - a few requests.

src/WorkspaceDependencyViewExtension/Properties/Resources.resx

QilongTang · 2019-10-11T19:35:55Z

I tried a bunch of ways to add hover over state, this way seems more consistent with what we have with the buttons for download. However I did notice that the buttons in dep viewer are not really consistent with what we have defined in Dynamo. Bummer. And refactoring isn't as easy as I imagined.. So I stopped here

mjkkirschner · 2019-10-14T15:37:06Z

@QilongTang LGTM

Initial Commit

ebd003c

mjkkirschner reviewed Oct 10, 2019

View reviewed changes

src/WorkspaceDependencyViewExtension/Properties/Resources.resx Outdated Show resolved Hide resolved

QilongTang added 2 commits October 10, 2019 11:50

Update tooltip

9d63e04

Refresh Button Hoverover State

79989c4

mjkkirschner added the LGTM Looks good to me label Oct 14, 2019

mjkkirschner approved these changes Oct 14, 2019

View reviewed changes

QilongTang merged commit e09bf01 into master Oct 14, 2019

QilongTang deleted the RefreshDepViewer branch October 14, 2019 18:00

Conversation

QilongTang commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Oct 10, 2019

Uh oh!

Uh oh!

QilongTang commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Oct 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

QilongTang commented Oct 10, 2019 •

edited

Loading

mjkkirschner Oct 10, 2019 •

edited

Loading

QilongTang commented Oct 11, 2019 •

edited

Loading