Add seriesOrder parameter (fixes #2663)

flosell commented 1 month ago

Description

Attempt to fix #2663 - let me know what you think

Checklist

[ ] ~For UI or styling changes, I have added a screenshot or gif showing before & after~
[x] I have added a changeset
[x] I have added to the docs where applicable
[ ] ~I have added to the VS Code extension where applicable~

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 7563909b7c562b1a9793b5313a11d0fdcdffca1e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

| Name | Type | | --------------------------------- | ----- | | @evidence-dev/component-utilities | Patch | | @evidence-dev/core-components | Patch | | my-evidence-project | Patch | | @evidence-dev/components | Patch | | evidence-test-environment | Patch | | e2e-prerender | Patch | | e2e-spa | Patch | | e2e-themes | Patch |

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

vercel[bot] commented 1 month ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 4, 2024 6:30pm
next-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 4, 2024 6:30pm

netlify[bot] commented 1 month ago

Deploy Preview for evidence-development-workspace ready!

Name	Link
Latest commit	7563909b7c562b1a9793b5313a11d0fdcdffca1e
Latest deploy log	https://app.netlify.com/sites/evidence-development-workspace/deploys/6729113e534c5d00082389a6
Deploy Preview	https://deploy-preview-2667--evidence-development-workspace.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 month ago

Deploy Preview for docs-evidence failed. Why did it fail? →

Name	Link
Latest commit	a7beb05ac6dc788b86d3bc9f7d59429fb26d9189
Latest deploy log	https://app.netlify.com/sites/docs-evidence/deploys/6717b2f1ed911f00071ffdd4

flosell commented 1 month ago

Saw the netlify errors above but couldn't find a reason for those to fail - the example project and docs build just fine locally (and at least the latter seem to also be fine deploying to vercel...). Any pointers? Or is this error unrelated to this PR?

archiewood commented 4 weeks ago

This appears to be failing CI in node 18

hughess commented 4 weeks ago

@flosell thanks for the PR! This looks great overall.

The test issues with node v18 look to be coming from toSorted (see here: https://stackoverflow.com/questions/76006398/tosorted-works-only-in-a-browser)

Could you swap this out for another method?

Otherwise this will be a great addition!

vercel[bot] commented 4 weeks ago

@flosell is attempting to deploy a commit to the Evidence Team on Vercel.

A member of the Team first needs to authorize it.

netlify[bot] commented 4 weeks ago

Deploy Preview for evidence-test-env ready!

Name	Link
Latest commit	7563909b7c562b1a9793b5313a11d0fdcdffca1e
Latest deploy log	https://app.netlify.com/sites/evidence-test-env/deploys/6729113ea1bc3100088cf8e3
Deploy Preview	https://deploy-preview-2667--evidence-test-env.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 4 weeks ago

Deploy Preview for next-docs-evidence ready!

Name	Link
Latest commit	7563909b7c562b1a9793b5313a11d0fdcdffca1e
Latest deploy log	https://app.netlify.com/sites/next-docs-evidence/deploys/6729113e025ba20008e11e0f
Deploy Preview	https://deploy-preview-2667--next-docs-evidence.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

flosell commented 4 weeks ago

Hi @hughess @archiewood, thanks for checking and sorry about that! Changed to in-place sort(), this should fix things

hughess commented 4 weeks ago

Awesome thanks @flosell !

hughess commented 4 weeks ago

@flosell it looks like the series order doesn't take effect in BarChart for some reason. All the other charts I've tested have worked though

archiewood commented 4 weeks ago

kicked off tests

flosell commented 3 weeks ago

@hughess good catch, thanks! Accidentally passed the argument to <Chart/> instead of <Bar/>. BubbleChart had the same problem by the way. Both should be fixed now

flosell commented 3 weeks ago

Hi @zachstence! Added some stories - this is my first time working with storybook so feel free to give feedback.

Also, if you expect folks to add stories when contributing, maybe add a few notes in CONTRIBUTING.md to save some review cycles?

Some notes on the change:

On some chart types (e.g. BubbleChart, ScatterPlot), I combined seriesColor and seriesOrder to make the effect of the seriesOrder a bit more visible. Let me know if you'd rather not mix multiple attributes in the same story
ScatterPlot didn't have a story file yet so I created one, inspired by BubbleChart (those two chart types seem to be very similar...)
I noticed chromatic doesn't seem to pick up the new stories - is that intentional or is there something I did wrong?

flosell commented 3 weeks ago

Also, sorry about the noise above - accidentally messed up the PR when rebasing and took a few tries to fix it :)

zachstence commented 2 weeks ago

@flosell Everything looks good!

I'm trying to figure out why Chromatic isn't snapshotting the new stories...

zachstence commented 2 weeks ago

I think Chromatic didn't snapshot your new stories because our GitHub action always clones the upstream evidence-dev/evidence repo, and not your fork. I'll look into the appropriate way to handle that.

Going to go ahead and merge this since it looks good! Thanks @flosell !

evidence-dev / evidence