Settings: Metric Configuration: Metric Details & Breakdowns (2/n)

[mxosman]

Description of the change

Continues iteration on the new Metric Configuration page and specifically implements the checkbox breakdowns (replacing the toggle switches) and sets up the Definitions (includes/excludes) panel.

Metric.Config.-.Breakdowns.mov

Related issues

Closes #72

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]

Using this arrow for now because it's much easier to implement.

[mxosman]

The logic didn't really change here - it's just now rendering the disaggregations based on the active tab and conforming to the new design (replaces toggle switches with checkboxes).

[mxosman]

Just added a wrapper here and added space for the definitions (includes/excludes) - nothing else changed.

[mxosman]

Temporary copy from the designs - will request new copy that's more relevant to the current implementation (without the frequency toggles).

[lilidworkin]

Thanks for all of these inline comments!

[hobuobi]

This is looking great!

1. I noticed that it was possible for a breakdown category to be active without any of the breakdowns below it being checked off. I think we should just tie the state of the category checkmark to the breakdowns beneath it:

• If at least one breakdown is checked off, the category is checked off
• If none are checked off, turn that category off.

2. We should skip straight to the definitions view for the metric you click on (e.g., Staff) once that's implemented!
3. Let's reduce the vertical height of the boxes on the All Metrics page. Ideally it would be responsive to the text inside, though I guess that makes aligning all the edges of the grid a bit harder... so for now, just reducing the height is good!
4. Is it possible to make sure that the full border shows up blue upon hover? I think right now it's partially obscured by the other boxes -- might just be a z-height thing.

[terryttsai]

This isn't a part of your PR but for the MetricConfiguration file, do we still need to call getDatapoints if we're not showing the charts?

[terryttsai]

Perhaps in the future we'll put components such as checkboxes, forms, buttons etc in something like a shared primitives components directory? Doesn't have to be the focus now.

[mxosman]

Yeah - when things feel less variable, I think there is plenty of work we can do with the styled components to reorganize and put into a shared library.

[terryttsai]

Yeah doesn't have to be for the current metric settings work, but perhaps for a new page / feature we can discuss organizing components and try putting new general components into the shared directory?

[mxosman]

Absolutely!!! Would also love to at some point do a sweep and merger of all our current "finalized" primitives to the new shared directory.

[lilidworkin]

Love this idea! Maybe we can make some time for this at the end of this sprint

[terryttsai]

In the MetricsView component in MetricConfiguration.tsx, do we need to keep track of both the metricSettings and filteredMetricSettings in state? I think we just need filteredMetricSettings?

[terryttsai]

Thought about it a bit more, so I'm thinking when we fetch metric settings we can set metric settings in state as usual, but for filteredMetricSettings we can just derive that from the metricSettings, we don't have to set filteredMetricSettings in state:

  type MetricSettingsObj = {
    [key: string]: MetricsViewMetric;
  };
  const [metricSettings, setMetricSettings] = useState<MetricSettingsObj>({});

  const filteredMetricSettings: MetricSettingsObj = Object.values(
    metricSettings
  )
    .filter(
      (metric) =>
        metric.system.toLowerCase() === activeMetricFilter?.toLowerCase()
    )
    ?.reduce((res: MetricSettingsObj, metric) => {
      res[metric.key] = metric;
      return res;
    }, {});

I put this right below const [activeMetricKey, setActiveMetricKey] = useState<string>("");

[mxosman]

Thank you for the feedback, Humphrey!

1. I noticed that it was possible for a breakdown category to be active without any of the breakdowns below it being checked off. I think we should just tie the state of the category checkmark to the breakdowns beneath it:

• If at least one breakdown is checked off, the category is checked off
• If none are checked off, turn that category off.

The current behavior sounds exactly like what you described -- sorry I might be confused! Current behavior is -- if no dimensions within a breakdown are turned on, then the checkmark on the overall breakdown (in the tab) will be unchecked/turned off. If at least one dimension within a breakdown is checked, then the overall breakdown is checked.

It behaves similar to the previous overall breakdown toggle (unchecking it/"turning it off" means you want to turn all of the dimensions off - similarly, if you turn all of the dimensions of a breakdown off, then the overall breakdown checkmark switches to unchecked/turns off).

Please let me know if I'm completely misunderstanding this! Is there's a timeframe in the video I can reference?

Here's an isolated video of the behavior in case its helpful to see by itself:

Screen.Recording.2022-10-11.at.4.51.01.PM.mov

2. We should skip straight to the definitions view for the metric you click on (e.g., Staff) once that's implemented!

Absolutely!

3. Let's reduce the vertical height of the boxes on the All Metrics page. Ideally it would be responsive to the text inside, though I guess that makes aligning all the edges of the grid a bit harder... so for now, just reducing the height is good!

Sounds good! What do you think of this?

Before:

After:

4. Is it possible to make sure that the full border shows up blue upon hover? I think right now it's partially obscured by the other boxes -- might just be a z-height thing.

It is! The boxes' border thickness is set to 1px. Mimicking the design - the boxes touch side by side and don't overlap each other which is why you'll see a 2px border in between the intersection of two boxes - but the hover state border the same as the border thickness - 1px.

Here's a close up:

Screen.Recording.2022-10-11.at.4.07.40.PM.mov

Let me know your thoughts!

[mxosman]

This isn't a part of your PR but for the MetricConfiguration file, do we still need to call getDatapoints if we're not showing the charts?

If the Data section won't appear within the MetricConfiguration, then no.

@hobuobi - a question that came up was where the Data Viz that was previously in the Metric Config section was going to live?

[mxosman]

In the MetricsView component in MetricConfiguration.tsx, do we need to keep track of both the metricSettings and filteredMetricSettings in state? I think we just need filteredMetricSettings?

Great catch! You're right - we don't need metricSettings and can just use filteredMetricSettings. Thanks for the optimization!

There's also plenty within this component that could use a lot of refactoring - and though I'm tempted to refactor everything, to better prioritize, I'm trying to make forward changes and only refactoring old code where necessary - until the very end (as time permits). Not sure if this is a good strategy - but it's helping me keep focused on updating the items that need to be changed, implementing new changes and do a full clean up (where I can also separate these components out into their own files) at the very end. 😄 Let me know your thoughts!

[terryttsai]

Yes, that generally sounds like a good strategy to me! I felt like this page is in active development so I didn't leave comments like breaking up the components into separate files for clarity. I left a comment on refactoring the file in the PR but it's not related to this specific PR so no need for that to block on this PR.

[mxosman]

Yes, that generally sounds like a good strategy to me! I felt like this page is in active development so I didn't leave comments like breaking up the components into separate files for clarity. I left a comment on refactoring the file in the PR but it's not related to this specific PR so no need for that to block on this PR.

That makes perfect sense - thanks for clarifying & for all of the feedback, Terry!

[terryttsai]

Yeah thanks for clarifying how you're prioritizing work too!

[lilidworkin]

This is all looking great to me! @mxosman / @terryttsai I think this general approach of how to handle refactoring and cleaning up makes sense. Generally I would encourage us to keep new code as clean as possible, and to "refactor as you write", basically. But I think it's totally fine to leave old code for a more dedicated refactoring sprint, to avoid getting distracted and pulled down a rabbit hole. Does that make sense?

@mxosman I'm also not sure what @hobuobi meant by the checkmark behavior -- it looks perfect to me!

[lilidworkin]

@mxosman actually quick question -- are these changes still hooked up to the old API? So like the changes you're making to the checkmarks are persisting like they used to?

[mxosman]

@mxosman actually quick question -- are these changes still hooked up to the old API? So like the changes you're making to the checkmarks are persisting like they used to?

Yeah - the behavior of the toggles and checkmarks is exactly the same - that logic went untouched. Mostly updated the CSS.