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: ajout des charts #300

Merged
merged 2 commits into from
Sep 8, 2024
Merged

feat: ajout des charts #300

merged 2 commits into from
Sep 8, 2024

Conversation

garronej
Copy link
Collaborator

@garronej garronej commented Sep 6, 2024

@garronej garronej marked this pull request as draft September 6, 2024 20:55
Copy link
Collaborator Author

@garronej garronej left a comment

Choose a reason for hiding this comment

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

Hey @K4ST0R,

Solid PR. Thank you for your contribution.

Please provide some Stories to guide pepole and maybe conduc some tests on Next in the test projects.
We can release a candidate when you're ready.

package.json Outdated
@@ -71,6 +71,7 @@
"@emotion/react": "^11.10.4",
"@emotion/styled": "^11.10.4",
"@gouvfr/dsfr": "1.12.1",
"@gouvfr/dsfr-chart": "^1.0.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Needs to go in dependencies, not devDependencies or it wont work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of that but all the other libraries are in "devDependencies" (not sure why). Maybe it is only use during the build ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, all dev dependencies are used only at build time and @gouvfr/dsfr is vendored because we need to apply some marginal transformation to it.

@@ -0,0 +1,42 @@
import React from "react";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
import React from "react";
"use client";
import React from "react";

This component is not RSC ready

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right!


/** @see <https://components.react-dsfr.codegouv.studio/?path=/docs/components-chart> */
export const BarChart = chartWrapper((props: BarChartBaseProps) => {
return <bar-chart {...stringifyObjectValue(props)} />;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is probably not Server Renderable and thus it won't work in next.js projects.

We'll have to instruct next user to use dynamic() https://nextjs.org/docs/pages/building-your-application/optimizing/lazy-loading#with-no-ssr

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, maybe it will be the case. I will test it and add a guide/example if this is the case :)

const graphProps = rest as T;

useEffect(() => {
prDsfrLoaded.then(() => setIsDsfrLoaded(true));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good job for figuring this on your own. I'm impressed

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! I was inspired by this : #244

@K4ST0R
Copy link
Contributor

K4ST0R commented Sep 7, 2024

I just added some test, storybook. 😁

I had to change some CSP rules to make chart works on next.

Also I fix issue with the next build.

@garronej garronej marked this pull request as ready for review September 7, 2024 21:53
@garronej
Copy link
Collaborator Author

garronej commented Sep 7, 2024

Great! Thanks a lot for your work!

Let's release!

@garronej garronej merged commit 0db478a into codegouvfr:main Sep 8, 2024
6 checks passed
@K4ST0R
Copy link
Contributor

K4ST0R commented Sep 8, 2024

Let's go!!

@garronej
Copy link
Collaborator Author

garronej commented Sep 8, 2024

You really did a great job on this.
Werry well executed and well polished PR!

Maybe, if you could go the extra miles and add a little notice on the storybook to instruct the Next user on how to use thoses components with dynamic().

Something like this:

*NOTE*: On small screens (mobile), you can click on the burger menu to open the menu modal.
You can watch if the menu modal is open or not with the \`useIsHeaderMenuModalOpen\` hook.

@garronej
Copy link
Collaborator Author

garronej commented Sep 9, 2024

Hey @K4ST0R,

Just to let you know, there's currently a dependency issue with @gouvfr/dsfr-chart so I've added it as devDependency for now.

I've created an issue: GouvernementFR/dsfr-chart#11

Maybe, if you could go the extra miles and add a little notice on the storybook to instruct the Next user on how to use thoses components with dynamic().

I've also done this.

Thanks again

@K4ST0R
Copy link
Contributor

K4ST0R commented Sep 9, 2024

Thank you a lot @garronej and thank you adding the notice :) !

Hmm that's an issue, I will check if i have time to create a PR in dsr-chart!

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