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

chore: Group Plotted Value with Univariate Measures (PT-186184994) #923

Merged

Conversation

emcelroy
Copy link
Contributor

@emcelroy emcelroy commented Oct 6, 2023

PT Story: https://www.pivotaltracker.com/story/show/186184994

Groups the Plotted Value graph adornment with Mean and Median so that it uses the same base component and model used by the latter.

Note that the following changes to the Plotted Value's appearance and behavior have been approved:

  • The placement of the Plotted Value's tool tip that appears on hover is at the same level that the Mean and Median's tool tips appear. (This differs from V2 where the Plotted Value's tool tip appears closer to the top of the line.)

  • The Plotted Value now supports the Show Measure Labels feature that's supported by Mean and Median.

  • The text that appears before the numeric value in the Plotted Value's tool tip and label will be value= (Similar to how the Mean's value is preceded by mean=, for example).

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (53118fe) 81.21% compared to head (287dccc) 81.12%.
Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
- Coverage   81.21%   81.12%   -0.10%     
==========================================
  Files         306      304       -2     
  Lines       12782    12703      -79     
  Branches     3472     3431      -41     
==========================================
- Hits        10381    10305      -76     
+ Misses       2278     2274       -4     
- Partials      123      124       +1     
Flag Coverage Δ
cypress 65.79% <89.18%> (-0.15%) ⬇️
jest 60.91% <52.63%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...src/components/graph/adornments/adornment-types.ts 100.00% <100.00%> (ø)
...s/univariate-measures/mean/mean-adornment-model.ts 91.66% <100.00%> (ø)
...s/univariate-measures/mean/mean-adornment-types.ts 100.00% <100.00%> (ø)
...ivariate-measures/median/median-adornment-model.ts 91.66% <100.00%> (ø)
...ivariate-measures/median/median-adornment-types.ts 100.00% <100.00%> (ø)
...iate-measures/plotted-value/edit-formula-modal.tsx 100.00% <100.00%> (ø)
...s/plotted-value/plotted-value-adornment-banner.tsx 100.00% <100.00%> (ø)
...ted-value/plotted-value-adornment-registration.tsx 100.00% <100.00%> (ø)
...res/plotted-value/plotted-value-adornment-types.ts 100.00% <100.00%> (ø)
...easures/univariate-measure-adornment-component.tsx 80.43% <100.00%> (+1.04%) ⬆️
... and 4 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emcelroy emcelroy force-pushed the 186184994-group-plotted-value-with-univariate-measure-adornments branch from 71428cc to b2275a9 Compare October 6, 2023 21:31
@emcelroy emcelroy force-pushed the 186184994-group-plotted-value-with-univariate-measure-adornments branch from b2275a9 to 7b4025f Compare October 6, 2023 21:33
@emcelroy emcelroy marked this pull request as ready for review October 6, 2023 21:48
@emcelroy emcelroy requested a review from kswenson October 6, 2023 21:48
Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Mostly this looks good. I'm not sure how to handle the strings changes. How was this handled in v2?

@@ -29,7 +31,7 @@ interface ILabelObject {
interface IProps {
cellKey: Record<string, string>
containerId?: string
model: IMeanAdornmentModel | IMedianAdornmentModel
model: IMeanAdornmentModel | IMedianAdornmentModel | IPlottedValueAdornmentModel
Copy link
Member

Choose a reason for hiding this comment

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

You may be able to just use IUnivariateMeasureModel here rather than having to list them all.

@emcelroy
Copy link
Contributor Author

👍 Mostly this looks good. I'm not sure how to handle the strings changes. How was this handled in v2?

@kswenson In v2 the Plotted Value's text label value is just the numeric value, there is no value= in front of it. I don't think you were present at the meeting, but I discussed the differences between the Plotted Value and Mean and Median with @bfinzer, which ones were important to keep, which ones might be open to change. One of the changes Bill thought should be made was to have value= before the number in the label.

Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

discussed the differences between the Plotted Value and Mean and Median with @bfinzer, which ones were important to keep, which ones might be open to change. One of the changes Bill thought should be made was to have value= before the number in the label.

👍 In that case, LGTM

@emcelroy emcelroy merged commit 6cce924 into main Oct 11, 2023
@emcelroy emcelroy deleted the 186184994-group-plotted-value-with-univariate-measure-adornments branch October 11, 2023 00:44
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.

2 participants