CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-8778 - alert user to login #16367
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-8778 - alert user to login #16367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8778
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces hardcoded “no suggestions” placeholders with dynamic ML-based titles and messages, introduces a user authentication flag for controlling placeholder visibility, and cleans up obsolete resources and minor documentation whitespace.
- Swap out
BoxPlaceholder
/NoItemsPlaceholder
forTitlePlaceholder
/MessagePlaceholder
with bindings and aDataTrigger
forDisplayAutocompleteMLStaticPage
- Add
IsUserAuthenticated
property in the ViewModel to gate ML content based on login state - Remove old resource entries and update core resource strings for consistency
- Clean up trailing blanks in the Node Autocomplete documentation HTML
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/NodeAutoCompleteViewExtension/Views/NodeAutoCompleteBarView.xaml | Update placeholders to bind to ML properties and add DataTrigger |
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs | Add IsUserAuthenticated and initial RaisePropertyChanged call |
src/DynamoCoreWpf/PublicAPI.Unshipped.txt | Remove obsolete AutocompleteNoSuggestions and AutocompletePlaceholder getters |
src/DynamoCoreWpf/Properties/Resources.resx & Resources.en-US.resx | Remove old placeholder entries |
src/DynamoCore/Properties/Resources.resx & Resources.en-US.resx | Change "No recommendations" title/message to new suggestion text |
src/DynamoCoreWpf/Controls/Docs/en-US/NodeAutocompleteDocumentation.html | Whitespace cleanup only |
Files not reviewed (1)
- src/DynamoCoreWpf/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (4)
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs:408
- There are no unit tests covering the new
IsUserAuthenticated
logic. Add tests to verify both authenticated and unauthenticated cases.
internal bool IsUserAuthenticated
src/DynamoCoreWpf/Controls/Docs/en-US/NodeAutocompleteDocumentation.html:1
- The documentation hasn't been updated to describe the new ML static page behavior or login alert. Consider adding a section explaining
DisplayAutocompleteMLStaticPage
and the new title/message placeholders.
<!--
src/DynamoCoreWpf/PublicAPI.Unshipped.txt:4549
- Removing these public resource getters is a breaking change. Ensure the API Changes document is updated to note this removal under Semantic Versioning guidelines.
static Dynamo.Wpf.Properties.Resources.AddStyleButton.get -> string
src/NodeAutoCompleteViewExtension/Views/NodeAutoCompleteBarView.xaml:10
- The
properties
XML namespace is added but never used in this XAML. Consider removing it to reduce clutter.
xmlns:properties="clr-namespace:Dynamo.Properties;assembly=DynamoCore"
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs
Outdated
Show resolved
Hide resolved
* fix cluster unlogged * undo html changes
Purpose
This PR replaces hardcoded “no suggestions” placeholders with dynamic ML-based titles and messages, introduces a user authentication flag for controlling placeholder visibility, and cleans up obsolete resources and minor documentation whitespace.
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
@DynamoDS/synapse
FYIs
@Jingyi-Wen