-
Notifications
You must be signed in to change notification settings - Fork 636
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-3657] Save changes label #11671
[DYN-3657] Save changes label #11671
Conversation
update master
update master
This reverts commit c78dfe9.
Update master
Update master
Master update from public repo
Update master
master update
Update master
Update master
Update master
Update master
Update branch
update master
Update Branch
Update master
Update master branch 4-3
Menu ux update
update master branch 12/3
Update branch
Update master branch
SavedChangesTooltip = string.Empty; | ||
} | ||
|
||
internal void UpdateSavedChangesLabel() |
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.
Let's add some comments to this function
@@ -87,6 +122,7 @@ public string SelectedLanguage | |||
{ | |||
selectedLanguage = value; | |||
RaisePropertyChanged(nameof(SelectedLanguage)); | |||
UpdateSavedChangesLabel(); |
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.
This call is repeated too many times. We can add a property change listener so we can listen to changes and call this automatically. That way we can get rid of all these single calls.
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.
Something like here: https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs#L636. Single handler can handle all property change I think, then we can filter out properties we need and call UpdateSavedChangesLabel()
there
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.
LGTM in general, it would be good that we can address the so many calls of UpdateSavedChangesLabel()
To avoid confusion on customer demo today, I am merging this in order to get rid of the save button. But @Astul-Betizagasti please address the PR comments using a new one. Also I checked the failure on self CI current failing on master branch already |
Purpose
Change the current Save Changes button on the Preferences modal into a label that will display information so the user can know when changes where saved
Includes a change to make the first expander on the Features tab be expanded by default when opening the Preferences modal
Declarations
Check these if you believe they are true
*.resx
filesReviewers
Aaron Tang (@QilongTang )
FYIs
Roberto Tellez (@RobertGlobant20 )