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

first draft of fiveThirtyEight theme #4

Merged
merged 11 commits into from
May 29, 2018
Merged

Conversation

Casyfill
Copy link
Contributor

@Casyfill Casyfill commented May 27, 2018

Style inspired by Charts from fivethirtyeight

based on:

@Casyfill
Copy link
Contributor Author

is it something with formatting?

@domoritz
Copy link
Member

Thank you so much for this PR. This is awesome.

Yes, just run yarn format and everything will be fine.

Can you add a screenshot of what it looks like now
?

@Casyfill
Copy link
Contributor Author

Casyfill commented May 28, 2018

Here is how it looks like right now - any suggestions are welcome;

https://beta.observablehq.com/@casyfill/538-theme-for-vega

screenshot 2018-05-28 23 09 53

@Casyfill
Copy link
Contributor Author

Casyfill commented May 28, 2018

going to fix

  • label / ticks overlapping
  • title

@domoritz
Copy link
Member

Sweet! I'm excited and happy to merge this when you feel it's in a good state to be released. We can keep iterating over time.

Would you want to become a maintainer of Vega-Themes? I can add you so you can review PRs and issues for the 538 theme.

@domoritz
Copy link
Member

Sorry for the strict formatting requirements. If you use the tslint and prettier extensions in your editor, things should format themselves automagically.

@Casyfill
Copy link
Contributor Author

it seems it does not accept filled for symbols - also, for symbols in legend. shall we add this feature?

@domoritz
Copy link
Member

it seems it does not accept filled for symbols - also, for symbols in legend. shall we add this feature?

In Vega?

@Casyfill
Copy link
Contributor Author

@domoritz both symbol: filled and line: strokeCap via linter - see travis logs.

ah yes, maintenance-vise PRs would be nice; if you're ok, feel free to merge this - I won't probably have time to tweak minor details during next week or two. Thank you!

@Casyfill
Copy link
Contributor Author

ah, one more thing - is that ok to name theme 538 or it might be a bad idea in terms of variable naming?

export { default as 538 } from './theme-fiveThirtyEight';

@domoritz
Copy link
Member

Let's name it fivethirtyeight instead

@domoritz
Copy link
Member

Also, can you check what you have done in #4 (comment)

rect: { fill: markColor },

range: {
category: ['#30a2da', '#fc4f30', '#e5ae38', '#6d904f', '#8b8b8b'],
Copy link
Member

Choose a reason for hiding this comment

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

Are there more than 5 colors?

Copy link
Contributor Author

@Casyfill Casyfill May 29, 2018

Choose a reason for hiding this comment

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

added 4 more + divergent and heatmap colormaps (see above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added 4 more colors and heatmap + divergent colormaps

@Casyfill
Copy link
Contributor Author

renamed as fivethirtyeight

@domoritz
Copy link
Member

Looks great!

@Casyfill
Copy link
Contributor Author

hm @domoritz can you explain that test fail, please?

@domoritz
Copy link
Member

If you rebase against master, this issue should be fixed.

@domoritz
Copy link
Member

Can you update the readme and make sure the example page works? I will merge then.

@Casyfill
Copy link
Contributor Author

Casyfill commented May 29, 2018

I think it is good to go - but I can't change your 'example' observable notebook - only fork from it; It's probably better if we change it on your side?

viewof theme = radio({
  title: 'Please select a theme',
  description: 'The charts below will update automatically.',
  options: [
    { label: 'Default', value: undefined },
    { label: 'Dark', value: 'dark' },
    { label: 'Microsoft Excel', value: 'excel' },
    { label: 'ggplot2', value: 'ggplot2' },
    { label: 'Quartz', value: 'quartz' },
    { label: 'Vox', value: 'vox' },
    { label: 'FiveThirtyEight', value: 'fivethirtyeight' }
  ],
  value: undefined
})

@domoritz
Copy link
Member

Can you change test/index.html?

@domoritz domoritz mentioned this pull request May 29, 2018
@domoritz
Copy link
Member

Let's rename the file to fivethirtyeight.ts.

screen shot 2018-05-29 at 09 29 19

I also noticed that the stroke width of symbols is a little too small. Let's use at least 1.

@domoritz domoritz merged commit 1b23214 into vega:master May 29, 2018
@domoritz
Copy link
Member

Fantastic. Thank you!

@Casyfill
Copy link
Contributor Author

hoorah,
thanks for the help and guidance!

@domoritz
Copy link
Member

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