feat(data-visualization): add package #2200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 04fc775 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
✔️ Deploy Preview for paste-theme-designer ready! 🔨 Explore the source changes: 04fc775 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-theme-designer/deploys/620d866194e82f000882f68e 😎 Browse the preview: https://deploy-preview-2200--paste-theme-designer.netlify.app |
TheSisb
commented
Feb 9, 2022
Comment on lines
+48
to
+49
| "highcharts": "^9.3.3", | ||
| "highcharts-react-official": "^3.1.0", |
There was a problem hiding this comment.
I mark these two as devDeps because I want to use them in the story file. Highcharts alone is a peerDep because we expect consumers to only need Highcharts to use the hook.
| } | ||
|
|
||
| return merge(options, { | ||
| colors: Object.values(context.dataVisualization), |
There was a problem hiding this comment.
We originally only designed for these sequential color values, but I took the liberty of applying our text and font styles elsewhere to make the charts look more branded. The nice thing about this package is we can update the styles as needed and ship it as a patch, which folks can get for free.
There was a problem hiding this comment.
Yeah, I was wondering about that actually when I looked at the story. Very worthwhile.
We will need one of the PD crew to review the text styles once we get the docs up. That design pass can happen at that time though
| * @param {Highcharts.Options} options - options to be provided into Highcharts for rendering. | ||
| * @returns {Highcharts.Options} optionsWithPasteTheme - Passed options with deepmerged Paste Theme | ||
| */ | ||
| export const usePasteHighchartsTheme = (options: Highcharts.Options): Highcharts.Options => { |
There was a problem hiding this comment.
I needed to take an options object as argument here because the theme in Highcharts is applied over the same object that configures chart data. I was running into issues where my styles weren't applying because the options object defined different values for certain keys. So we need to take the options object, deepmerge our styles over them, and return a new object which is passed into Highcharts. This works pretty well, as can be seen in the stories.
| * @param {Highcharts.Options} options - options to be provided into Highcharts for rendering. | ||
| * @returns {Highcharts.Options} optionsWithPasteTheme - Passed options with deepmerged Paste Theme | ||
| */ | ||
| export const usePasteHighchartsTheme = (options: Highcharts.Options): Highcharts.Options => { |
There was a problem hiding this comment.
We need this to be a React hook so that the styles update upon toggling between light and dark theme. Can't just export a static object, unless we instruct developers to swap it themselves which is much less ergonomic and more error prone.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e984ef1:
|
|
Size Change: +222 B (0%) Total Size: 466 kB
ℹ️ View Unchanged
|
andioneto
reviewed
Feb 9, 2022
packages/paste-libraries/data-visualization/stories/index.stories.tsx
Outdated
Show resolved
Hide resolved
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
TheSisb
force-pushed
the
add-data-visualization-pkg
branch
from
ac421f0
to
86fe68e
Compare
SiTaggart
reviewed
Feb 9, 2022
SiTaggart
reviewed
Feb 10, 2022
packages/paste-libraries/data-visualization/stories/index.stories.tsx
Outdated
Show resolved
Hide resolved
TheSisb
force-pushed
the
add-data-visualization-pkg
branch
from
86fe68e
to
857e51d
Compare
|
✔️ Deploy Preview for paste-docs ready! 🔨 Explore the source changes: 04fc775 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-docs/deploys/620d8661e7dd580008d1a5e4 😎 Browse the preview: https://deploy-preview-2200--paste-docs.netlify.app |
SiTaggart
approved these changes
Feb 16, 2022
There was a problem hiding this comment.
Code looks fine. I'm more than fine with copy pasta examples from highcharts too with ts-ignores.
One final thing, which I didn't spot, we will need to add highcharts as a root dependency because Storybook is a root application. Right now it works when import highcharts into a story because yarn is hoisting the dep from the data visualisation package to the root of the repo.
TheSisb
force-pushed
the
add-data-visualization-pkg
branch
2 times, most recently
from
9403ec1
to
793863d
Compare
gloriliale
approved these changes
Feb 16, 2022
|
A few things:
@katieporter I chatted with Shadi, and since this PR is really just to add the tokens, with a few fixes to fix really obvious things, he's going to create a ticket for followup work to polish up the theme with more design-eng pairing. We can address these comments ^ when that polish work happens. |
TheSisb
force-pushed
the
add-data-visualization-pkg
branch
3 times, most recently
from
dd050a4
to
d01b2d2
Compare
TheSisb
force-pushed
the
add-data-visualization-pkg
branch
from
d01b2d2
to
e984ef1
Compare
zahnster
approved these changes
Feb 16, 2022
serifluous
approved these changes
Feb 16, 2022
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new package called
@twilio-paste/data-visualization-library. This package will serve as a container for future data viz utilities. In its current form, we export a helpful React hook which exports a Highcharts option object with Paste theme values applied. This will help developers build themed and accessible charts in Highcharts more easily. I've added a couple stories showing how it works.