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.

Sign up for GitHub

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

Jump to bottom

Fix statistics positions #4146

Merged
merged 3 commits into from Oct 23, 2023
Merged

Fix statistics positions #4146

merged 3 commits into from Oct 23, 2023

Conversation

Bestem0r
Copy link
Contributor

Description

  • Displays the statistics charts as two grid columns instead of one.
  • Adds card background to all charts.

Result

Before After
Screenshot from 2023-09-27 12-22-54 Screenshot from 2023-09-27 12-22-16

Testing

  • I have thoroughly tested my changes.
  1. Navigate to an event and click on "Se påmeldinger".
  2. Click on "Statistikk".
  3. Verify that the new design uses a two column grid and that all charts have a card background.

Resolves ABA-497

@Bestem0r Bestem0r added the enhancement Pull requests that make enhancements, instead of just purely new features label Sep 27, 2023
@github-actions github-actions bot added the review-needed Pull requests that need review label Sep 27, 2023
ivarnakken
ivarnakken requested changes Sep 27, 2023
View reviewed changes
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Looks like an improvement!🙌🏼
Just some nit-picking

app/routes/events/components/EventAttendeeStatistics.tsx Outdated Show resolved Hide resolved
app/routes/events/components/EventAttendeeStatistics.tsx Outdated Show resolved Hide resolved
app/routes/events/components/EventAttendeeStatistics.tsx Outdated Show resolved Hide resolved
app/routes/events/components/EventAttendeeStatistics.tsx Show resolved Hide resolved
app/routes/events/components/EventAttendeeStatistics.tsx Outdated Show resolved Hide resolved
app/routes/events/components/EventAttendeeStatistics.tsx Outdated Show resolved Hide resolved
falbru
falbru approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@falbru falbru left a comment

Choose a reason for hiding this comment

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

LGTM 🔥. Although you should use the Card component instead, like Ivar suggested.

Also remember to squash before you merge!!

itsisak
itsisak approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@itsisak itsisak left a comment

Choose a reason for hiding this comment

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

Looks great!!

danielyanghansen
danielyanghansen requested changes Sep 28, 2023
View reviewed changes
Copy link
Member

@danielyanghansen danielyanghansen left a comment

Choose a reason for hiding this comment

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

Fix linting errors and remember to rebase with master to fix cypress tests failing

ivarnakken
ivarnakken reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Almost there! hehe

app/routes/events/components/EventAttendeeStatistics.css Outdated Show resolved Hide resolved
app/routes/events/components/EventAttendeeStatistics.css Outdated Show resolved Hide resolved
app/routes/events/components/EventAttendeeStatistics.tsx Outdated Show resolved Hide resolved
@Bestem0r
Copy link
Contributor Author

Bestem0r commented Oct 4, 2023

Should be good now @ivarnakken @danielyanghansen

ivarnakken
ivarnakken reviewed Oct 4, 2023
View reviewed changes
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Just a tiny tiny comment left hehe

app/routes/events/components/EventAttendeeStatistics.tsx Outdated Show resolved Hide resolved
danielyanghansen
danielyanghansen approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@danielyanghansen danielyanghansen left a comment

Choose a reason for hiding this comment

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

Same comments as Ivar.

Looks good! 💯

@ivarnakken
Copy link
Member

Seems you had a little accident when rebasing. Please try again 😄

@Bestem0r
Wrap statistics inside cards
01fdf95 
Extend bottom chart, reformat with prettier

Replace custom card with dedicated card component, remove inline styling

Remove card color, improve styling

Reformat with prettier

Rebase branch 'master' onto fix-statistics-positions

Replace card titles with card header component

Reformat with prettier

Wrap statistics inside cards

Extend bottom chart, reformat with prettier

Replace custom card with dedicated card component, remove inline styling

Remove card color, improve styling

Reformat with prettier

Wrap statistics inside cards

Extend bottom chart, reformat with prettier

Replace custom card with dedicated card component, remove inline styling

Remove card color, improve styling

Wrap statistics inside cards

Extend bottom chart, reformat with prettier

Replace custom card with dedicated card component, remove inline styling

Remove card color, improve styling

Reformat with prettier

Replace card titles with card header component

Reformat with prettier

Rebase branch 'master' onto fix-statistics-positions

Attempt to fix rebasing issues

Reformat with prettier
ivarnakken
ivarnakken approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Awesome!

@ivarnakken ivarnakken added the approved Pull requests that have been approved label Oct 23, 2023
@ivarnakken ivarnakken merged commit e3b4f29 into master Oct 23, 2023
4 checks passed
@ivarnakken ivarnakken deleted the fix-statistics-positions branch October 23, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsisak itsisak itsisak approved these changes

@danielyanghansen danielyanghansen danielyanghansen approved these changes

@falbru falbru falbru approved these changes

@ivarnakken ivarnakken ivarnakken approved these changes

@AdrianAndersen AdrianAndersen Awaiting requested review from AdrianAndersen

@ShaileshS1702 ShaileshS1702 Awaiting requested review from ShaileshS1702

Assignees
No one assigned
Labels
approved Pull requests that have been approved enhancement Pull requests that make enhancements, instead of just purely new features review-needed Pull requests that need review
Projects
None yet
Milestone
No milestone
5 participants
@Bestem0r @ivarnakken @itsisak @danielyanghansen @falbru