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: Update Basic Grouped Bar Chart Legend #371

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

lee00678
Copy link
Collaborator

@lee00678 lee00678 commented Apr 16, 2024

Quick update for the legend to only capitalize the first letter. (Issue)

Please see screenshot:
Screenshot 2024-04-17 at 10 57 06 AM

@lee00678 lee00678 requested a review from reb-dev April 16, 2024 21:41
@rokotyan
Copy link
Contributor

Thanks for fixing this @lee00678!

I've a small thought: since it is one of our official examples, I find it a little "dirty" to use n.charAt(0).toUpperCase() + n.slice(1) there. In the real word use case, I would define a function to do this, or used text-transform: capitalize in CSS.

I don't think we want to overcomplicate this example by adding unnecessary CSS. We can probably add a "capitalize" function to data.ts or simply add a labels map there: { republican: 'Republican', ... }. Let me know what you think.

P.S. We usually have a practice of adding screenshots and videos to our PRs to simplify the review process. That would be great if you could do it too.

@lee00678
Copy link
Collaborator Author

Thanks for fixing this @lee00678!

I've a small thought: since it is one of our official examples, I find it a little "dirty" to use n.charAt(0).toUpperCase() + n.slice(1) there. In the real word use case, I would define a function to do this, or used text-transform: capitalize in CSS.

I don't think we want to overcomplicate this example by adding unnecessary CSS. We can probably add a "capitalize" function to data.ts or simply add a labels map there: { republican: 'Republican', ... }. Let me know what you think.

Agree with not adding CSS to complicate the example. I like the idea of adding a capitalize function in data.ts. I think it's a common use case and would be nice to provide an small example for that? I will update. Thanks for the feedback!

P.S. We usually have a practice of adding screenshots and videos to our PRs to simplify the review process. That would be great if you could do it too.

Gotcha.

@lee00678 lee00678 force-pushed the qian/update-grouped-bar-legend-39 branch from 157a508 to f6ac5d3 Compare April 17, 2024 17:56
@rokotyan
Copy link
Contributor

Yep, that works! Thanks for addressing the comments.

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. Thanks @lee00678

@lee00678 lee00678 merged commit 0999a98 into main Apr 25, 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