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

Improve plots ribbon block tooltips #2956

Merged
merged 4 commits into from
Dec 19, 2022
Merged

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Dec 16, 2022

  • highlight table keys based off type
  • add copy button to table values

Demo

demo.mov

Followup to #2924, Fixes #2457

* highlight table keys based off type
* add copy button to table values
@julieg18 julieg18 added the product PR that affects product label Dec 16, 2022
@julieg18 julieg18 self-assigned this Dec 16, 2022
@julieg18 julieg18 marked this pull request as ready for review December 16, 2022 16:57
@julieg18 julieg18 requested a review from shcheklein December 16, 2022 16:59
@shcheklein
Copy link
Member

shcheklein commented Dec 16, 2022

Amazing improvement!

QQ - when it a number, does the copy button copy an actual value? Just to double check? And same with data hash ...

@julieg18
Copy link
Contributor Author

julieg18 commented Dec 16, 2022

when it a number, does the copy button copy an actual value? Just to double check? And same with data hash ...

When you say the "actual value", are you referring to the value that isn't truncated? If so, yes!

path: path.slice(type.length + 1) || path,
type,
Copy link
Member

Choose a reason for hiding this comment

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

[Q] Why not send only the raw data from the model and have the client(s) format it? See webview/src/experiments/util/buildDynamicColumns.tsx for how this is handled by the experiments table.

Copy link
Member

Choose a reason for hiding this comment

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

If not there is some commonality with ExperimentsTree.getTooltipTable. The duplication should be removed/combined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely would simplify the code a lot! Will do it in a followup :)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@codeclimate
Copy link

codeclimate bot commented Dec 19, 2022

Code Climate has analyzed commit bdbe132 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.6% (0.0% change).

View more on Code Climate.

@julieg18 julieg18 merged commit ee28d1c into main Dec 19, 2022
@julieg18 julieg18 deleted the improve-plot-exp-block-tooltips branch December 19, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve experiments naming
4 participants