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

Website | Gallery: Donut Example #38 #367

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Website | Gallery: Donut Example #38 #367

merged 1 commit into from
Apr 12, 2024

Conversation

lee00678
Copy link
Collaborator

Add a gallery example which showcases the use of a basic donut chart. (issue)

  • Create a folder in shared/examples directory called basic-donut-chart with all the relevant files
  • Find dataset and create example in basic-donut-chart.tsx

Add code for other frameworks:

  • TypeScript
  • Angular
  • Svelte
  • Vue

And then add screenshots to _shared/previews:

  • basic-donut-chart.png
  • basic-donut-chart-dark.png

@lee00678 lee00678 requested a review from reb-dev April 10, 2024 20:20
@rokotyan
Copy link
Contributor

Great job @lee00678!

I'm a little concerned about the visual aspects of the new example because it's inconsistent with other examples and our Unovis brand. Specifically, the colors and the capitalized text in the legend.
image

@Dovlash Vlad, can you please take a look and give us your comments. Here's the Gallery section on our website if you need a link: https://unovis.dev/gallery. (Vlad is leading our data visualization design activities at F5)

@rokotyan
Copy link
Contributor

@reb-dev It might be a good time to enforce the Commit Lint formats: #300

@lee00678 lee00678 force-pushed the qian/add-donut-#38 branch from abc0227 to 9d2770f Compare April 10, 2024 21:32
@lee00678
Copy link
Collaborator Author

Great job @lee00678!

I'm a little concerned about the visual aspects of the new example because it's inconsistent with other examples and our Unovis brand. Specifically, the colors and the capitalized text in the legend.

I'm happy to make any changes! Also nice to "meet" both of you @rokotyan and @Dovlash. Yes, I used D3's color theme as I ran out of the default colors for donut chart. I can probably look into how to get unovis' color theme rendered, and I will update the legend as well.

@rokotyan
Copy link
Contributor

rokotyan commented Apr 10, 2024

Nice to meet you too @lee00678!

I see, I think instead of showing so many slices, we can merge smaller ones into a big "Others" one.
Here are two great posts about Pie Charts and Colors in data vis:

@lee00678 lee00678 force-pushed the qian/add-donut-#38 branch from 9d2770f to 815292f Compare April 11, 2024 17:33
@lee00678
Copy link
Collaborator Author

lee00678 commented Apr 11, 2024

I see, I think instead of showing so many slices, we can merge smaller ones into a big "Others" one. Here are two great posts about Pie Charts and Colors in data vis:

I like the idea of combining the smaller categories. Made an update on that.

@rokotyan
Copy link
Contributor

Nice, thanks for the update!

@rokotyan
Copy link
Contributor

@lee00678 Btw, if you use Discord, we've a space there which we're planning to make public soon. Feel free to join: https://discord.gg/zATXusWgCP

@lee00678 lee00678 force-pushed the qian/add-donut-#38 branch from 815292f to e0fb1d5 Compare April 11, 2024 19:21
@lee00678 lee00678 self-assigned this Apr 11, 2024
Copy link
Collaborator

@reb-dev reb-dev left a comment

Choose a reason for hiding this comment

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

Looks good @lee00678! I left one comment, I think you'll need to rebase on main to address it

@@ -25,6 +25,13 @@ export const examples: ExampleCollection[] = [
require('./horizontal-stacked-bar-chart').default,
],
},
{
title: 'Donut Charts',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of making a new category, I would add it as the first entry to "Circular Charts"

@lee00678 lee00678 force-pushed the qian/add-donut-#38 branch from e0fb1d5 to 1ceb62a Compare April 12, 2024 02:46
Copy link

github-actions bot commented Apr 12, 2024

CLA Assistant Lite bot ✅ All required contributors have signed the F5 CLA for this PR. Thank you!

@lee00678
Copy link
Collaborator Author

CLA Assistant Lite bot: 🎉 Thank you for your contribution. It appears you have not yet signed the F5 Contributor License Agreement (CLA), which is required for your changes to be incorporated into an F5 project. Please kindly read the F5 CLA and comment the following to agree:

I have hereby read the F5 CLA and agree to its terms

Qian Liu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.You can retrigger this bot by commenting recheck in this Pull Request

Agree

@lee00678 lee00678 force-pushed the qian/add-donut-#38 branch from 1ceb62a to c9e210e Compare April 12, 2024 02:57
Copy link
Collaborator

@reb-dev reb-dev left a comment

Choose a reason for hiding this comment

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

Thanks @lee00678! Merging.

@reb-dev reb-dev merged commit ba9096a into main Apr 12, 2024
4 checks passed
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.

3 participants