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 5288 node autocomplete improvements #13410

Merged
merged 13 commits into from
Oct 25, 2022
Merged

Dyn 5288 node autocomplete improvements #13410

merged 13 commits into from
Oct 25, 2022

Conversation

jesusalvino
Copy link
Contributor

@jesusalvino jesusalvino commented Oct 19, 2022

Purpose

Implement the story https://jira.autodesk.com/browse/DYN-5288

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

FYIs

@RobertGlobant20 @filipeotero

@jesusalvino
Copy link
Contributor Author

Case 1: no results (0 items)

uno

Case 2: the result has at least one item assuming each node could be by recommendation or by use

dos

Case 3: Confidence score are under a threshold, assuming the minimum value is 50 and some result nodes are under it

tres

Case 4: Confidence score are under a threshold, assuming the minimum value is 50 and all results nodes are under it

cuatro

@Amoursol
Copy link
Contributor

@jesusalvino looking awesome as a first pass! A couple of comments:

  • Overall, is the font used Artifakt for consistency with the rest of the UI? Looks different to my naked eye, but I could be wrong.
  • For "Case 2", is there any reason we keep the graphic? The message states there are now nodes, but we are showing two options. This will be confusing to end-users.
  • We should also have at the very least a "Watch node" as an output option to all nodes.

@jesusalvino
Copy link
Contributor Author

@Amoursol I have already discussed with @Jingyi-Wen about the Case 2:

Assuming that each node from the ML result could be "by use" (recently used) or "by recommendation", this case shows 2 results and they are only by use, that's why the "no recommendations message" is displayed, it probably rarely happens, please let me know if we are good with the implementation of the mockups, any change / update is welcome.

@Amoursol
Copy link
Contributor

@Amoursol I have already discussed with @Jingyi-Wen about the Case 2:

Assuming that each node from the ML result could be "by use" (recently used) or "by recommendation", this case shows 2 results and they are only by use, that's why the "no recommendations message" is displayed, it probably rarely happens, please let me know if we are good with the implementation of the mockups, any change / update is welcome.

Ah, that makes sense. In this case, please do keep it as is!

/// <summary>
/// Indicates if display the Low confidence option and Tooltip
/// </summary>
public bool DisplayLowConfidence { get; set; }
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 all of these properties? It seems most of them will have same/opposite value of Models.NodeAutocompleteSuggestion.MLRecommendation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, according to the Mockups we have different messages and behavior based on the type of the results: by use, recommendations and confidence score.

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.

Looks good but I have two questions so wondering if we can further simplify/ specify things

}
RaisePropertyChanged(nameof(DisplayNoRecommendationsLowConfidence));
Copy link
Contributor

Choose a reason for hiding this comment

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

First I feel these better not be raised manually but part of the setter so they are automatic?
Also this function PopulateAutoCompleteCandidates() is called a lot but do we need to set these values all the time? Can we just set these values everytime when user pick the autocomplete method?

get
{
return dynamoViewModel.PreferenceSettings.DefaultNodeAutocompleteSuggestion == Models.NodeAutocompleteSuggestion.MLRecommendation;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

removed setter and changed the view code to be one way binding

@QilongTang QilongTang added this to the 2.17.0 milestone Oct 24, 2022
@QilongTang
Copy link
Contributor

@jesusalvino After my changes, user should be able to switch back and forth as well, see below:

SwitchNodeAutoCompleteOption

@QilongTang
Copy link
Contributor

Got approval from @jesusalvino on slack. Merging in for follow up changes.

@QilongTang QilongTang merged commit 80e427d into DynamoDS:master Oct 25, 2022
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