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

ComboBox. Support parameterless constructor. Update scenario demo #607

Merged
merged 24 commits into from
Jun 4, 2020
Merged

ComboBox. Support parameterless constructor. Update scenario demo #607

merged 24 commits into from
Jun 4, 2020

Conversation

fergusonr
Copy link
Contributor

No description provided.

@tig
Copy link
Collaborator

tig commented Jun 3, 2020

Nice work.

See: #447

I was just reviewing all event code throughout the project and noted this:

  • public event EventHandler<ustring> Changed;
    • The implementation has some issues:
      1. The name is "Changed" but the event actually represents "the selection has been confirmed". Rename it to SelectionChanged?
      2. It's sending the current text; best practice is for an event to provide previous value. I'm not sure which is more appropriate for this API, but if you KNOW users of the event are going to want the 'new' text, then keep it as is. If you are not sure, then my suggestion is to defined a SelectionChangedEventArgs class and include both the old and new value in it.
      2. Calls to Invoke are spread around. Ideally there would be public virtual void OnSelectionChanged() method that calls SelectionChanged?.Invoke.

@tig tig merged commit 254b779 into gui-cs:master Jun 4, 2020
@tig
Copy link
Collaborator

tig commented Jun 4, 2020

LGTM!

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