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

update metric settings badges to reflect custom reporting frequency #172

Merged

Conversation

nichelle-hall
Copy link
Contributor

@nichelle-hall nichelle-hall commented Nov 21, 2022

Description of the change

This PR updates the badges used in the metric settings pages so that they reflect the custom reporting frequency of a metric if the custom frequency is not undefined.

The default reporting frequency for Civilians Complaints Sustained is annual, but a custom reporting frequency has been added such that it is now reported monthly.

Screen Shot 2022-11-21 at 2 05 31 PM

Screen Shot 2022-11-21 at 2 05 33 PM

Related issues

Related to #16218

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

@nichelle-hall nichelle-hall force-pushed the nichelle-hall/metrics-settings-reporting-frequency branch from f646fe0 to bac641f Compare November 21, 2022 20:04
@nichelle-hall nichelle-hall requested review from mxosman, terryttsai, lilidworkin and morden35 and removed request for mxosman and terryttsai November 21, 2022 20:07
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.

@nichelle-hall can you do a search to see if there's anywhere else we need to update the rendering of frequency? I just remembered for instance that the badge also shows up on the Data page. Will your change here take care of that, or do we need to make an additional change for that?

@@ -358,6 +361,8 @@ class MetricConfigStore {
this.metrics[systemMetricKey].description = metadata.description;
this.metrics[systemMetricKey].frequency =
metadata.frequency as ReportFrequency;
this.metrics[systemMetricKey].customFrequency =
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious what @mxosman and @terryttsai think, but I'm wondering if we should define a property like frequencyToUse here, which is customFrequency if not null else frequency, and then just use this everywhere when rendering. That way, up above we could replace

                    {metrics[systemMetricKey]?.customFrequency
                      ? metrics[
                          systemMetricKey
                        ]?.customFrequency?.toLocaleLowerCase()
                      : metrics[systemMetricKey]?.frequency?.toLowerCase()}

with

{metrics[systemMetricKey]?.frequencyToUse?.toLowerCase()}

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like that idea - love how clean it makes rendering the frequency! Let me know if TypeScript gives you any trouble, Nichelle!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to leave the default frequency because I wasn't sure if it would be included as part of the design. For example, if the user wanted to see what the default was while scrolling through, or wanted to set every metric back to default. That said I think Humphrey's designs will come soon! I won't let this be blocking for more than a couple days but will finish up the other follow-ups first before doing these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, I wonder if we would want to include the default in the data tab design since we may need to distinguish that the reporting frequency has changed if we want to change the x axis of the charts.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a great call - makes sense why you'd need them separated in the cases you mentioned!

Copy link
Contributor

@mxosman mxosman left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, Nichelle!

@lilidworkin
Copy link
Collaborator

Sounds good, thanks for explaining your reasoning @nichelle-hall !

@nichelle-hall nichelle-hall force-pushed the nichelle-hall/metrics-settings-reporting-frequency branch from bac641f to 2af960c Compare November 28, 2022 23:51
@nichelle-hall nichelle-hall merged commit af54d51 into main Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants