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

assign colors to activities and remove Absence #606

Merged
merged 5 commits into from
Aug 31, 2022

Conversation

HenrikeW
Copy link
Contributor

@HenrikeW HenrikeW commented Aug 30, 2022

Related issue(s) and PR(s)

This PR closes #591

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

List of changes made

  • all activities used in 2021 and 2021 have now a color assigned
  • in case an unknown activity is fetched from the database (which shouldn't happen...) there is an escape color
  • Absence is excluded from the chart

Screenshot of the fix

Testing

  • go to the report page and look at your bar chart. The biggest categories should have distinct colors, there shouldn't be any two very similar colors next to each other (I really hope that's the case, but it's hard to test as I only see my own reporting. So please let me know if there is anything weird!)

Further comments

Definition of Done checklist

  • I have made an effort making the commit history understandable
  • I have performed a self-review of my own code and commented any hard-to-understand areas
  • Tests and lint/format validations are passing
  • My changes generate no new warnings

@HenrikeW HenrikeW requested a review from a team August 30, 2022 08:58
Copy link
Contributor

@jonandernovella jonandernovella left a comment

Choose a reason for hiding this comment

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

Looks fine! I left just a small nitpicker

frontend/src/components/BarChart.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@jonandernovella jonandernovella left a comment

Choose a reason for hiding this comment

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

I think the activities should not be hardcoded, but instead generate the object based on the existing fetched activities.

frontend/src/components/BarChart.tsx Outdated Show resolved Hide resolved
KattisLej
KattisLej previously approved these changes Aug 30, 2022
Copy link
Contributor

@KattisLej KattisLej left a comment

Choose a reason for hiding this comment

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

If it's possible to change according to the comments before then I think it's a good idea. Otherwise it looks good.

@KattisLej KattisLej dismissed their stale review August 30, 2022 11:48

Found a potential bug

@KattisLej
Copy link
Contributor

KattisLej commented Aug 30, 2022

I found this, double Internal NBIS. It seems to be added multiple times. Maybe it should a bug of its own:
image
image

@HenrikeW
Copy link
Contributor Author

I found this, double Internal NBIS. It seems to be added multiple times. Maybe it should a bug of its own: image image

I can't reproduce this. I've tried adding several time entries for several issues with the activity "Internal NBIS" in several weeks, but everything gets added up as it's supposed to. Would be really interesting to see if anyone else sees the bug. Could you try?

@ghost
Copy link

ghost commented Aug 30, 2022

Hi. I also get a duplicate for "Internal NBIS". It shows 3 hours for each, but says 20% for one and 17% for the other. This is on the dev/user_groups branch. Posted image/screenshot in Slack.
The duplicate disappears after reload, but the numbers shown are not correct. Time reported for current week is not included if after "today".
Steps to reproduce duplicate:

  1. Add two rows with same activity for different issues.
  2. Enter value (today or before today) on one of the issues. Save changes.
  3. Enter a value for the other issue with same activity. Save again.
  4. Enter one more value (for another day) for one of the issues and save again. The bar now gets a duplicate, one showing the old sum and one showing the new.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Good idea to keep the names as comments!

@HenrikeW
Copy link
Contributor Author

The color scheme is now based on activity ids instead of activity names. That required a slightly more complicated structure for the spentTime variable as we still need to store the activity names somewhere to then display them as labels in the bar chart.

That new structure would have made the alphabetical sorting of the categories in the bar chart a lot more complicated. Anyhow, IIRC the main point of having an alphabetical order was to have some consistent order. With the updated data structure, the order will be consistent, sorted by activity id (because JS by default orders integer-like object keys in ascending order). I figured, this is fine instead of having alphabetical order, considering the fact that the code gets much shorter this way.

@HenrikeW HenrikeW requested review from jonandernovella, KattisLej and a user August 31, 2022 09:28
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Ordering by id is fine.

@jonandernovella jonandernovella merged commit 4d36f88 into develop Aug 31, 2022
@jonandernovella jonandernovella deleted the dev/update-color-scale branch August 31, 2022 10:43
@jonandernovella jonandernovella mentioned this pull request Sep 6, 2022
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.

assign colors to activities in bar chart
3 participants