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.

Sign up for GitHub

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

Jump to bottom

Race & Ethnicities (Playtesting Followup): Debounce Saving & Send All Dimensions #174

Merged
merged 1 commit into from Nov 30, 2022
Merged

Race & Ethnicities (Playtesting Followup): Debounce Saving & Send All Dimensions #174

merged 1 commit into from Nov 30, 2022

Conversation

mxosman
Copy link
Contributor

@mxosman mxosman commented Nov 23, 2022 •
edited

Description of the change

Debounce saving and send all 24 dimensions instead of the deltas. Also, optimize saving by using inputs for the buttons and utilizing onChange.

Demo: https://playtesting---justice-counts-web-qqec6jbn6a-uc.a.run.app/

Related issues

Contributes to #180

Checklists

Development

This box MUST be checked by the submitter prior to merging:

  • Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

  • Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@mxosman mxosman changed the title Race & Ethnicities (Playtesting Followup): Optimize Saving Race & Ethnicities (Playtesting Followup): Debounce Saving Nov 28, 2022
@mxosman mxosman changed the title Race & Ethnicities (Playtesting Followup): Debounce Saving Race & Ethnicities (Playtesting Followup): Debounce Saving & Send All Dimensions Nov 28, 2022
@mxosman mxosman marked this pull request as ready for review November 29, 2022 17:22
Instead of saving deltas - save all 24 dimensions
96b56f2 
Further optimize by using inputs and onChange to prevent multiple calls when clicking on the active option

Debounce saving

Extend binary input radio buttons to mini buttons
@mxosman mxosman requested a review from a team November 29, 2022 20:12
lilidworkin
lilidworkin approved these changes Nov 30, 2022
View reviewed changes
Copy link
Collaborator

@lilidworkin lilidworkin left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for making this change. It actually seems to simplify the code a bit too!

Also, optimize saving by using inputs for the buttons and utilizing onChange.

Just out of curiosity, would you mind expanding on this a little?

publisher/src/components/Forms/BinaryRadioButton.tsx
...props
}): JSX.Element => {
return (
<RadioButtonWrapper>
<RadioButtonWrapper lastOptionBlue={lastOptionBlue}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this lastOptionBlue change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the designs, it looks like the mini buttons are all grey when selected, except for the last button which is blue when selected.
Screenshot 2022-11-30 at 10 43 51 AM

Since I'm extending our BinaryRadioButton component, I needed to add this to have it deviate from it's current behavior throughout the app (all selected options are blue):

Screenshot 2022-11-30 at 10 43 04 AM

Screenshot 2022-11-30 at 10 42 58 AM

Let me know if you think I should have a clearer name/add a comment in the BinaryRadioButton component describing this.

@mxosman
Copy link
Contributor Author

mxosman commented Nov 30, 2022

Looks great! Thanks for making this change. It actually seems to simplify the code a bit too!

Thank you for the suggestion - it definitely simplified a lot of the code!

Also, optimize saving by using inputs for the buttons and utilizing onChange.

Just out of curiosity, would you mind expanding on this a little?

Absolutely! Before I just used a div to act as a mini button and used an onClick handler - every time you clicked on an option that was already selected, it would fire off the onClick function. Here, I am extending the current binary radio input component and using the input's onChange event handler so nothing fires off when the user clicks an already selected option. It's better practice in general to be using inputs for buttons. Since our buttons are continuing to evolve, I'll make a task to go through at some point and centralize/clean them up.

@mxosman
Copy link
Contributor Author

mxosman commented Nov 30, 2022

I'll merge this in as is for right now - but happy to make further adjustments in follow up PRs!

@mxosman mxosman merged commit b7c01c6 into main Nov 30, 2022
@mxosman mxosman deleted the mahmoud/re-optimize-saving branch November 30, 2022 17:54
@lilidworkin
Copy link
Collaborator

Nice, that makes perfect sense @mxosman ! Thanks for explaining!

terryttsai
terryttsai reviewed Nov 30, 2022
View reviewed changes
publisher/src/stores/MetricConfigStore.tsx
return {
key: metricKey,
disaggregations: [
{
key: RACE_ETHNICITY_DISAGGREGATION_KEY,
dimensions: updatedDimensions,
dimensions,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if dimensions is undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

If in line 817 raceEthnicitiesDimensions is falsey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always a great question - part of the reason I added the raceEthnicitiesDimensions and dimensions below the updater function is to ensure that dimensions will always have a value. In fact, dimensions doesn't even really need the raceEthnicitiesDimensions && check.

If somehow it's undefined and we get to this point, it'll just return the object without the dimensions property - but I can't figure out how it could end up as undefined by the time we get here in either function.

terryttsai
terryttsai reviewed Nov 30, 2022
View reviewed changes
publisher/src/stores/MetricConfigStore.tsx
return {
key: metricKey,
disaggregations: [
{
key: RACE_ETHNICITY_DISAGGREGATION_KEY,
dimensions: updatedDimensions,
dimensions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, what if dimensions is undefined?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terryttsai terryttsai terryttsai left review comments

@lilidworkin lilidworkin lilidworkin approved these changes

Assignees
No one assigned
Labels
Type: Non-breaking refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mxosman @lilidworkin @terryttsai