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

Add 'Remember' CheckBox to the LoginDialog #2308

Merged
merged 3 commits into from
Jan 16, 2016
Merged

Add 'Remember' CheckBox to the LoginDialog #2308

merged 3 commits into from
Jan 16, 2016

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".

}

public class LoginDialogData
{
public string Username { get; set; }
public string Password { get; set; }
public bool Remember { 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.

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