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

Issues/KUI 1196 show barchart labels #356

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

amirhossein-haerian
Copy link
Contributor

For this task me and @axelbjo added the name of each school below each bar in the charts.
After conversation with Joel:

  1. we added name of school next to year and period
  2. we changed the chart heading font-size to “font-heading-xs”

@axelbjo
Copy link
Contributor

axelbjo commented Jun 20, 2024

Looks good to me!

Copy link
Contributor

@belanglos belanglos 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 to me! Good job 🚀


const seasonsStr = seasons.map(season => seasonLib.labelSeason(season, languageIndex)).join(', ')
return (
<Row>
<Col xs="4" style={{ flex: 'none', width: 'auto', paddingBottom: '20px' }}>
<label>{translation.statisticsLabels.formLabels.formSubHeaders.year}</label>
<label>{formSubHeaders.school}</label>
{`: ${school === 'allSchools' ? statisticsLabels[school] : school}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

I always have a hard time reading ternary operators inline like that. What do you think of extracting that part to something like getSchoolName or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We liked the idea and we made the required changes 🚀

@amirhossein-haerian amirhossein-haerian merged commit aeccc99 into master Jun 24, 2024
3 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