Skip to content

Conversation

manekovskiy
Copy link
Contributor

This adds a "Remember ..." checkbox to the LoginDialog. Reference feature #2305.

To keep backward compatibility the Checkbox is Collapsed by default.
The default text for Checkbox equals to "Remember".

Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the name of this property to a more boolean-sounding name. I would suggest something like IsRememberRequested, RequestRemember. This is because the developer might be confused because it might understand that MahApps does remember the state of the dialog.
Another possibility would be to add a documentation tag to the property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ShouldRemember is better since it's a request to the caller to persist the data.

Additionally I'd suggest to make all setters of LoginDialogData internal. There's no need to set these properties outside of MahApps.Metro.

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, I was also thinking about ShouldRemember as alternative to Remember. It is more explicit.
Totally agree on suggested internal improvement.

Made all property setters of LoginDialogData internal.
@punker76 punker76 added this to the 1.3.0 milestone Jan 12, 2016
punker76 added a commit that referenced this pull request Jan 16, 2016
…e-LoginDialog

Add 'Remember' CheckBox to the LoginDialog
@punker76 punker76 merged commit cded951 into MahApps:develop Jan 16, 2016
punker76 added a commit that referenced this pull request Jan 18, 2016
…e-LoginDialog

Add 'Remember' CheckBox to the LoginDialog (reverted from commit cded951)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants