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

[big number] improve visual aesthetics of big number visualization #4313

Closed
wants to merge 3 commits into from

Conversation

williaster
Copy link
Contributor

@williaster williaster commented Jan 30, 2018

I planted this seed a while ago and think it's time we start some easy wins on improving the visual aesthetics of some visualizations (especially those that are widely used), so hacked on it over the weekend.

It does

  • use the @data-ui/xy-chart package
  • handling long-number/subheader edge cases, which the current visualizations do not (see gif below).

It does not

  • show colors for + or - trends, but I would argue that the previous colors were not visually appealing and we can add that in a future PR if it's a big request.
  • show an x- or y-axis, but you can explore all data using the tooltip (which displays date + value).

big-number-improvements

@mistercrunch @hughhhh @graceguo-supercat @michellethomas @elibrumbaugh

TODO before merge

  • fix tooltip bounds detection
  • add support for mapping comparison changes to color

@williaster
Copy link
Contributor Author

FYI some of our power users do have strong feelings about the color comparison so I'll update with tweaks there.

const TEXT_SIZE_SCALAR = 0.1;

// Returns the maximum font size (<= idealFontSize) that will fit within the available Width
function getMaxFontSize({ text, availableWidth, idealFontSize, fontWeight = 'normal' }) {
Copy link
Member

@hughhhh hughhhh Jan 31, 2018

Choose a reason for hiding this comment

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

nit: Can we use arrow functions?

const getMaxFontSize = () => {...}

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 think it's generally preferable to define helper functions as normal functions as opposed to arrow functions because if there’s an error in function myHelper then the function name will appear in the stack trace whereas const myHelper will show up as an anonymous function.

I think arrow function style is preferable when you can take advantage of the syntax to make the function definition more succinct (like const getDate = d => d.date) but beyond that it seems like only upside to declaring a normal function.

const Visualization = (
<div style={{ height: totalHeight, ...CONTAINER_STYLES }}>
<div
style={{
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would use a const var or potentially styles.json file. I can see us have an data-ui directory just like the deckgl dir and creating components specifically around data-ui

Would love to here your thoughts @williaster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A data-ui dir is an interesting idea. I think a lot of these styles are still specific to big number, though, and are mostly targeted around dynamic font sizing. For instance here you can't use a const var beyond BIG_NUMBER_STYLES because the fontSize is dynamically computed

@hughhhh
Copy link
Member

hughhhh commented Jan 31, 2018

This is an awesome start by the way!

@mistercrunch
Copy link
Member

Nice. In the gif it looks for a second like the tooltip isn't overflowing correctly on the edge of the widget (over half a second, so I can't tell for sure).

@williaster
Copy link
Contributor Author

^good eye! debugging it locally still, updated the PR with todo items before it's ready for prime time.

@williaster
Copy link
Contributor Author

closing this in favor of #5469

@williaster williaster closed this Jul 24, 2018
@williaster williaster deleted the chris--bignumber branch July 24, 2018 17:41
@williaster williaster mentioned this pull request Jul 24, 2018
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.

3 participants