Adding Dynamo Dictionary link to Context Menu Help Popup #7572

yeexinc · 2017-02-03T09:48:34Z

Purpose

This PR is to address DYN-306
A link to Dynamo Dictionary is added at the bottom of help menu popup window.

If the size of the content is longer than the default window size, user has to scroll down to see the link. E.g.

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

@aparajit-pratap

FYIs

@riteshchandawar @Racel

Racel · 2017-02-03T13:03:41Z

@yeexinc - Does the link go to the node's page in the dictionary or just to the front page? If it is just to the front page, that is fine for now. But, in the future, we should file another task to get the link pointing to the node's page. Is that doable?

yeexinc · 2017-02-05T01:56:43Z

@Racel , for now it only takes the user to the front page. Though I do agree that going to the node's page would make it a better feature.

aparajit-pratap · 2017-02-06T05:47:35Z

@yeexinc as discussed you can easily link each node to its own help page in DynamoDictionary as suggested by @Racel . You would need to bind to the Category property of NodeModel to get the category information and use it to construct the path to be appended to the DynamoDictionary website.
For example: for the ByGeometryColor node, its Category property returns the string Display.Display.Display.Create. You can use this information together with the node name to reconstruct the path and append it to the website name such that you can reconstruct the complete URL, which will navigate the user directly to the node's page: http://dictionary.dynamobim.com/#/Display/Display/Display/Create/ByGeometryColor

yeexinc · 2017-02-06T09:24:42Z

Thank you for the review @aparajit-pratap , the new changes should direct the user to the node's help page now.
Some nodes have blank strings as their Category however, such as the Watch node and the Web Request node. In such cases the user will be directed to the Dynamo Dictionary's home page for now.

aparajit-pratap · 2017-02-06T13:47:23Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

+
+                // Obtain the node's default name from its creation name.
+                // e.g. For creation name DSCore.Math.Max@double,double - the name "Max" is obtained and appended to the final link.
+                int indexAfter = (CreationName.LastIndexOf('@') == -1) ? CreationName.Length : CreationName.LastIndexOf('@');


I think there should be an easier more straightforward way to retrieve the node name. Try using the Nickname or Name property of NodeModel.

Thanks @aparajit-pratap , it appears that some nodes have empty strings as their Name (an example is AnalysisExtensions.IsAlmostEqualTo). Though, the actual name is still obtainable from CreationName, something which I don't quite understand as CreationName seems to return the node's Name property from their get method (line 77 of the code in this file).
Nickname changes accordingly with the UI when the user renames the node. Hence, I think that CreationName gives a better accuracy among the three.

aparajit-pratap · 2017-02-06T13:48:42Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

+                string finalLink = Configurations.DynamoDictionary + "#/";
+                if (category == null || category == "")
+                {
+                    return finalLink; // if there is no category, return the link to home page


Why don't you simply return Configurations.DynamoDictionary. That way you won't have to wrap this up in a try-catch below, would you?

I was concerned that ((TextBlock)sender).Tag.ToString() in NodeHelpPrompt.xaml.cs may throw some unexpected errors, but that seems unlikely in this case. Will fix that in my next commit. Thanks!

aparajit-pratap · 2017-02-06T13:50:02Z

@yeexinc please take a look at my comments and give them some thought. Thanks!

Racel · 2017-02-06T13:54:48Z

@aparajit-pratap @yeexinc - This is great that you got some of the nodes pointing to the correct dictionary location. Please keep in mind overloads where the category/node model approach might not be enough.

Take for instance, Point.ByCoordinates. In the dictionary, the first overload works as outlined in the approach @aparajit-pratap suggested. The subsequent overloads use the additional parameters as part of the url...http://dictionary.dynamobim.com/#/Geometry/Point/Create/ByCoordinates(x_double-y_double-z_double)

aparajit-pratap · 2017-02-07T04:17:48Z

@Racel in the case of overloads therefore, the link may be misleading as it will always lead the user to the first overload. @riteshchandawar any thoughts?

Racel · 2017-02-07T13:06:23Z

@aparajit-pratap - I agree, we should not lead the user to the first overload. Is it not possible to just use similar logic to what is used for the dictionary? Checking with ModeLab about this...

Racel · 2017-02-07T20:07:16Z

@aparajit-pratap @yeexinc - Please check out line 273 of https://github.com/ekatzenstein/DynamoDictionary_React/blob/master/src/App.js for routing logic setup in dictionary

Racel · 2017-02-13T21:23:32Z

@aparajit-pratap @yeexinc - Just checking in on this? How is it going? And are we ready to merge?

yeexinc · 2017-02-14T09:51:01Z

@Racel - We were still looking into the issue with overloads.
May I know if there is a rule to determine which method requires parameters in the URL? I've thought of taking the first overload among the functions, or the overload with the least/most number of parameters as the "default" method which do not require parameters in the URL, but the Dynamo Dictionary does not seem to follow the pattern.
Any ideas? Or, is it possible to have all the methods with overloads to have parameters in their URL?
Thanks.

Racel · 2017-02-14T16:12:22Z

@yeexinc - I think the overloads are loaded alphabetically. So, the first overload alphabetically will not have the parameters in the URL.

See if that works...

yeexinc · 2017-02-15T04:01:14Z

@Racel - Checking the overloads alphabetically works for some cases, but not all. Take "Point.ByCoordinates" as an example, the overloads for this method in alphabetical order are Point.ByCoordinates(x,y) and Point.ByCoordinates(x,y,z). This will generate valid URLs, since only the subsequent overload contains parameters in its URL.
In the case of "SortIndexByValue" though, the overloads, in alphabetical order are SortIndexByValue(list) and SortIndexByValue(list, ascending). However, the parameters are appended in the first overload instead of the subsequent overload.

It would be good if there is a rule to decide which function requires parameters and which does not, for consistency.

(Note: operations to append parameters are not included in this commit)

aparajit-pratap · 2017-02-16T03:26:15Z

@Racel I had written an email to Erick from Modelab to ask if they could consistently add parameters to all overloads in the DynamoDictionary website. Could you confirm with him if that's possible?

…ch-dictionary

yeexinc · 2017-02-17T06:54:11Z

@aparajit-pratap - The current state of this PR will always direct the user to the first overload, with no parameters added, as adding parameters will take the user to a blank page if the URL is invalid. It can be merged if you think it is okay for the time being.

aparajit-pratap · 2017-02-18T04:27:07Z

Thanks @yeexinc please cross-merge into RC1.2.3_master branch as well.

yeexinc · 2017-02-20T01:41:16Z

Thank you for the review @aparajit-pratap !

…1.2.3_master (#7620) * Added Dynamo Dictionary Link to Context Menu Help Popup * Redirect user to the node's page * Updated codes based on previous review (Note: operations to append parameters are not included in this commit) * Reverted NodeHelpPrompt.xaml * Reupdate NodeHelpPrompt.xaml * Removed JsonIgnore for RC1.2.3

Added Dynamo Dictionary Link to Context Menu Help Popup

e994b0e

Redirect user to the node's page

c33bc91

aparajit-pratap reviewed Feb 6, 2017

View reviewed changes

yeexinc added 4 commits February 15, 2017 12:29

Updated codes based on previous review

3df84ce

(Note: operations to append parameters are not included in this commit)

Reverted NodeHelpPrompt.xaml

5f1de75

Merge branch 'master' into branch-dictionary

e047a80

Reupdate NodeHelpPrompt.xaml

36e538e

Merge remote-tracking branch 'refs/remotes/DynamoDS/master' into bran…

5a7da2b

…ch-dictionary

aparajit-pratap merged commit e59d651 into DynamoDS:master Feb 18, 2017

yeexinc mentioned this pull request Feb 20, 2017

Cross merge #7572 (Dictionary links on context help menu popup) to RC1.2.3_master #7620

Merged

yeexinc mentioned this pull request Feb 24, 2017

Bug fix for dictionary links on Context Menu Help Popup #7637

Merged

7 tasks

yeexinc deleted the branch-dictionary branch March 14, 2017 03:09

yeexinc mentioned this pull request Mar 14, 2017

Test cases for dictionary link generation #7706

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Dynamo Dictionary link to Context Menu Help Popup #7572

Adding Dynamo Dictionary link to Context Menu Help Popup #7572

yeexinc commented Feb 3, 2017

Racel commented Feb 3, 2017

yeexinc commented Feb 5, 2017

aparajit-pratap commented Feb 6, 2017 •

edited

yeexinc commented Feb 6, 2017

aparajit-pratap Feb 6, 2017

yeexinc Feb 7, 2017 •

edited

aparajit-pratap Feb 6, 2017

yeexinc Feb 7, 2017

aparajit-pratap commented Feb 6, 2017

Racel commented Feb 6, 2017

aparajit-pratap commented Feb 7, 2017

Racel commented Feb 7, 2017

Racel commented Feb 7, 2017

Racel commented Feb 13, 2017

yeexinc commented Feb 14, 2017

Racel commented Feb 14, 2017

yeexinc commented Feb 15, 2017

aparajit-pratap commented Feb 16, 2017

yeexinc commented Feb 17, 2017

aparajit-pratap commented Feb 18, 2017

yeexinc commented Feb 20, 2017

Adding Dynamo Dictionary link to Context Menu Help Popup #7572

Adding Dynamo Dictionary link to Context Menu Help Popup #7572

Conversation

yeexinc commented Feb 3, 2017

Purpose

Declarations

Reviewers

FYIs

Racel commented Feb 3, 2017

yeexinc commented Feb 5, 2017

aparajit-pratap commented Feb 6, 2017 • edited

yeexinc commented Feb 6, 2017

aparajit-pratap Feb 6, 2017

Choose a reason for hiding this comment

yeexinc Feb 7, 2017 • edited

Choose a reason for hiding this comment

aparajit-pratap Feb 6, 2017

Choose a reason for hiding this comment

yeexinc Feb 7, 2017

Choose a reason for hiding this comment

aparajit-pratap commented Feb 6, 2017

Racel commented Feb 6, 2017

aparajit-pratap commented Feb 7, 2017

Racel commented Feb 7, 2017

Racel commented Feb 7, 2017

Racel commented Feb 13, 2017

yeexinc commented Feb 14, 2017

Racel commented Feb 14, 2017

yeexinc commented Feb 15, 2017

aparajit-pratap commented Feb 16, 2017

yeexinc commented Feb 17, 2017

aparajit-pratap commented Feb 18, 2017

yeexinc commented Feb 20, 2017

aparajit-pratap commented Feb 6, 2017 •

edited

yeexinc Feb 7, 2017 •

edited