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

feat: Show category counts/percentages on ValueCounts hover #43

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

jwilber
Copy link
Contributor

@jwilber jwilber commented Aug 16, 2024

Great project idea!

This PR adds in some UX seen in the Observable Summary Table, where hovering over values in the ValueCountsPlot displays the corresponding row count/percentage (see demo of PR in action in video below).

Observable Example:

quakPRObservable.mov

Quak Example (this PR):

quakPRHoverRowCount.mov

Two small differences here versus Observable:

  1. I format the percentage via d3.format(".1%"), while they appear to use d3.format(".0%"). (A future improvement could use a ternary to check if the percentage is less than 1, if so, format via d3.format(".1%") or .2,, otherwise, d3.format(".0%").

  2. On mouseout, they appear to show the JS data-type, while I show the type provided by the arrow.Field (as that's what was displayed by default in quak).

Note:

  • This PR also brings in some linting/formatting changes, which occurred after running deno lint + deno fmt.
  • This PR only affects the Value Counts plots (for categorical data). I can happily add another PR for the histogram as well 😄

@manzt
Copy link
Owner

manzt commented Aug 16, 2024

Love it! Thanks for the contribution. I'll give a review tomorrow.

re: deno fmt... not sure what's going on there. My guess is that you may have some editor "format on save" setting, which likely uses prettier (creating new line indentation, etc) and and then the deno fmt formats that new-line delimited code with dprint.

@jwilber
Copy link
Contributor Author

jwilber commented Aug 16, 2024

sgtm! I just went ahead and reset the committed linting changes.

Copy link
Owner

@manzt manzt 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! Stellar PR. Thanks again for the contribution.

@manzt manzt merged commit 90da659 into manzt:main Aug 16, 2024
3 checks passed
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