-
Notifications
You must be signed in to change notification settings - Fork 68
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 user label settings page #1481
Add user label settings page #1481
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.
LGTM! Just a couple of very minor points
if (result.deleted) { | ||
// It exists in our list and was deleted, so remove it. | ||
setState(() => userLabels.removeWhere((userLabel) => userLabel.username == result.userLabel!.username)); | ||
} else { | ||
// It exists in our list but was changed, so update it. | ||
setState(() => userLabels[userLabels.indexOf(existingLabel)] = result.userLabel!); | ||
} |
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 is more of a preference thing (so no need to change anything!), but I generally like to return early on these types of boolean conditions to reduce the logic. Of course, this only works if this is the last thing that needs to be performed.
if (result.deleted) { | |
// It exists in our list and was deleted, so remove it. | |
setState(() => userLabels.removeWhere((userLabel) => userLabel.username == result.userLabel!.username)); | |
} else { | |
// It exists in our list but was changed, so update it. | |
setState(() => userLabels[userLabels.indexOf(existingLabel)] = result.userLabel!); | |
} | |
if (result.deleted) { | |
// It exists in our list and was deleted, so remove it. | |
setState(() => userLabels.removeWhere((userLabel) => userLabel.username == result.userLabel!.username)); | |
return; | |
} | |
// It exists in our list but was changed, so update it. | |
setState(() => userLabels[userLabels.indexOf(existingLabel)] = result.userLabel!); |
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.
That makes sense! In this case, since there's only one line of logic in the else
, I think the parallelism of having both statements in conditions helps to see what's going on without too much clutter.
Pull Request Description
This PR adds a settings page to manage user labels.
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
qemu-system-x86_64_jxnmwGj9lh.mp4
Checklist
semanticLabel
s where applicable for accessibility?