- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.3k
Feat: Implement dynamic Pie Chart for Client Diversity and enhance localization support. #16547
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
base: dev
Are you sure you want to change the base?
Conversation
| ✅ Deploy Preview for ethereumorg ready!
 To edit notification comments on pull requests, go to your Netlify project configuration. | 
| Hello @wackerow, I effected the changes with the direction you gave earlier, thanks. However, I noticed this breaking since the push from about 11 hours ago on the dev which I pulled from. The error is not from the files I worked on, which I have tested locally already. Thanks. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LifeofDan-EL! Looking better =)
Left some suggestions. These charts can be tricky to get them "responsive" on all screen sizes, so I think the simpler-the-better here. Otherwise I think this looks good to go once those updates are made.. thanks again for the help!
| <ClientDiversityChart> | ||
| <PieChart | ||
| data={[ | ||
| { name: "Geth", value: 41 }, | ||
| { name: "Nethermind", value: 38 }, | ||
| { name: "Besu", value: 16 }, | ||
| { name: "Erigon", value: 3 }, | ||
| { name: "Reth", value: 2 } | ||
| ]} | ||
| title="Execution Clients" | ||
| /> | ||
|  | ||
| <PieChart | ||
| data={[ | ||
| { name: "Lighthouse", value: 42.71 }, | ||
| { name: "Prysm", value: 30.91}, | ||
| { name: "Teku", value: 13.86}, | ||
| { name: "Nimbus", value: 8.74}, | ||
| { name: "Lodestar", value: 2.67 }, | ||
| { name: "Grandine", value: 1.04 }, | ||
| { name: "Other", value: 0.07 } | ||
| ]} | ||
| title="Consensus Clients" | ||
| /> | ||
| </ClientDiversityChart> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
 
Getting challenging overflow issues with these charts when trying to lay them side-by-side. I think for simplicity and robustness, we should probably just keep them stacked.
Benefits:
- No x-overflow when they're side-by-side
- The ClientDiversityChartwrapper becomes completely unnecessary... can just render the twoPieChartcomponents we need
- Can also just place the sub-text directly here as markdown, avoiding the need for altering page-developers-docs.json
| <ClientDiversityChart> | |
| <PieChart | |
| data={[ | |
| { name: "Geth", value: 41 }, | |
| { name: "Nethermind", value: 38 }, | |
| { name: "Besu", value: 16 }, | |
| { name: "Erigon", value: 3 }, | |
| { name: "Reth", value: 2 } | |
| ]} | |
| title="Execution Clients" | |
| /> | |
| <PieChart | |
| data={[ | |
| { name: "Lighthouse", value: 42.71 }, | |
| { name: "Prysm", value: 30.91}, | |
| { name: "Teku", value: 13.86}, | |
| { name: "Nimbus", value: 8.74}, | |
| { name: "Lodestar", value: 2.67 }, | |
| { name: "Grandine", value: 1.04 }, | |
| { name: "Other", value: 0.07 } | |
| ]} | |
| title="Consensus Clients" | |
| /> | |
| </ClientDiversityChart> | |
| <PieChart | |
| data={[ | |
| { name: "Geth", value: 41 }, | |
| { name: "Nethermind", value: 38 }, | |
| { name: "Besu", value: 16 }, | |
| { name: "Erigon", value: 3 }, | |
| { name: "Reth", value: 2 } | |
| ]} | |
| title="Execution Clients" | |
| /> | |
| <PieChart | |
| data={[ | |
| { name: "Lighthouse", value: 42.71 }, | |
| { name: "Prysm", value: 30.91}, | |
| { name: "Teku", value: 13.86}, | |
| { name: "Nimbus", value: 8.74}, | |
| { name: "Lodestar", value: 2.67 }, | |
| { name: "Grandine", value: 1.04 }, | |
| { name: "Other", value: 0.07 } | |
| ]} | |
| title="Consensus Clients" | |
| /> | |
| > This diagram may be outdated—go to [ethernodes.org](https://ethernodes.org) and [clientdiversity.org](https://clientdiversity.org) for up-to-date information. | 
 
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in the index.md file, would just ditch this wrapper entirely, along with it's import in MdComponents/index.tsx, and the additions to page-developers-docs.json... that string can go straight into the markdown file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have effected the changes. Testing before sending out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LifeofDan-EL! Overall looking good... I made a small adjustment to help keep as much as possible in the markdown, and a couple tiny patches to the punctuation used in the copy.
These are so tricky to get just right, and responsive on all screen sizes. @pettinarip Curious if you have any thoughts on ways to improve this... on desktop it leaves a lot of space, but adjusting seems to end up cutting things off at other screen sizes. Right now this seems to be at a reasonable middle ground where it's.. not perfect, but at least readable on all device sizes.
Another thing I'll note is the link out to "Michael Sproul" which is a repo marked as no longer being accurate since Electra. Would be nice to find an alternative, but doesn't need to block this PR imo.
Approving, but @pettinarip let me know if you have any thoughts before pulling this in 🙏 Thanks folks!
| Perfectly understood @wackerow, will keep an eye out for any future needs and suggestions. I do agree that getting it to be perfectly responsive is quite tricky. | 

Description
This pull request addresses an update to the website's client diversity section by introducing a new PieChart component with the following key changes:
Dynamic Data Visualization: The chart now renders dynamically, updating the visualization based on the provided data values.
Component Reusability: The PieChart component has been abstracted and made reusable, allowing it to be easily implemented in other parts of the application as needed.
Internationalization (i18n) Support: All text elements within the client diversity chart are now configured for localization, facilitating future language translation efforts.
Related Issue
Fixes #15099