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

SKS chart - WIP ⚒️ #458

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

SKS chart - WIP ⚒️ #458

wants to merge 17 commits into from

Conversation

mikolaj-jalocha
Copy link
Member

  • for clarity the chart shows only 6 periods of the given time, 3 in the past, 3 in the future
  • didn't test in on high numbers like 100, but should scale well
  • any thing else we can add here? Maybe some note about from where the data comes from? Btw wdyt about making an article about sks's chart data on our blog? How it started, who's behind etc. as a fun fact :-) We could then link to that from our app.
chart #378

@mikolaj-jalocha
feat: api-setup
a7a133b
@mikolaj-jalocha
feat: temp navigation setup
69789f0
@mikolaj-jalocha
chore: apply linter rules
a047687
@mikolaj-jalocha
feat: api-setup
55f0e1f
@mikolaj-jalocha
feat: temp navigation setup
c43ec62
@mikolaj-jalocha
chore: apply linter rules
344365f
@mikolaj-jalocha
feat: add sks chart
d64669b
@mikolaj-jalocha
feat: add sks chart
2140657
@mikolaj-jalocha
Merge remote-tracking branch 'origin/feat/sks-chart' into feat/sks-chart
32f6098 
# Conflicts:
#	lib/features/sks_chart/data/models/sks_chart_data.dart
#	lib/features/sks_chart/data/repository/sks_chart_repository.dart
#	lib/features/sks_chart/presentation/sks_chart_screen.dart
#	lib/features/sks_people_live/presentation/widgets/sks_user_data_button.dart
#	lib/utils/datetime_utils.dart
@mikolaj-jalocha
chore: code cleanup
044969d
@mikolaj-jalocha
Merge branch 'main' of https://github.com/Solvro/mobile-topwr into fe…
0ed0bea 
…at/sks-chart

# Conflicts:
#	lib/l10n/app_pl.arb
@mikolaj-jalocha
chore: code separation
3b736e8
@mikolaj-jalocha
chore: remove navigation
3f495f0
@mikolaj-jalocha
chore : final formatting
a4e3526
@mikolaj-jalocha
chore : change sks menu package name to reflect convention
ae92b4e
@mikolaj-jalocha
chore : change fl chart version & fix linter complaints
d5b93aa
@mikolaj-jalocha mikolaj-jalocha added the enhancement New feature or request label Nov 30, 2024
@mikolaj-jalocha mikolaj-jalocha self-assigned this Nov 30, 2024
@mikolaj-jalocha mikolaj-jalocha marked this pull request as ready for review November 30, 2024 20:55
@simon-the-shark
Copy link
Member

Hey! Didn't look more into this, but I have some quick thoughts/suggestions about the design:

  1. vertical scale labels maybe should be on the left side - like in parking chart to be consistent??
  2. I think we need at least 4h scope of time for it to be useful
  3. We can have few times more bars - just the labels don't need to be for every one of them. I think when we have more dense bars, this should look better. Maybe more like in the Google Maps? I thinks it's pretty neatly done there.
  4. maybe some small and vague horizontal lines?? - quite optional
  5. maybe smaller corner radius of the bars??????????
  6. maybe let's put the whole chart into some greyish container? - to be more consistent with the parkings?

not about the chart: maybe we should also show current people counter and trend in the bottom drawer too? cause the app bar one is kinda in the background. In the parkings we have the current number too, so it would be more consistent.

Tell me what you think! Hopefully I don't interfere with your vision too much 🦔

@tomasz-trela
Copy link
Member

I think vertical bars should be at right site. Similar to parkings. I think interval should be bigger.

@tomasz-trela
Copy link
Member

Hey! Didn't look more into this, but I have some quick thoughts/suggestions about the design:

  1. vertical scale labels maybe should be on the left side - like in parking chart to be consistent??
  2. I think we need at least 4h scope of time for it to be useful
  3. We can have few times more bars - just the labels don't need to be for every one of them. I think when we have more dense bars, this should look better. Maybe more like in the Google Maps? I thinks it's pretty neatly done there.
  4. maybe some small and vague horizontal lines?? - quite optional
  5. maybe smaller corner radius of the bars??????????
  6. maybe let's put the whole chart into some greyish container? - to be more consistent with the parkings?

not about the chart: maybe we should also show current people counter and trend in the bottom drawer too? cause the app bar one is kinda in the background. In the parkings we have the current number too, so it would be more consistent.

Tell me what you think! Hopefully I don't interfere with your vision too much 🦔

Oh I didn't see your message.

@mikolaj-jalocha
Copy link
Member Author

Extremely fast visualization of your propositions:

  • imo gray background doesn't look good
  • certain hours (ie. each 3rd or even 4th) could be placed obliquely to the graph)
  • live counter is a good option, I will implement it
  • smaller corner radius's good idea
  • Do you think that left axis need title?
  • with 4 hours of scope (5 min intervals) chart is not readable. In following scenario prob. line chart'd be better.
chart

@tomasz-trela
Copy link
Member

Extremely fast visualization of your propositions:

  • imo gray background doesn't look good
  • certain hours (ie. each 3rd or even 4th) could be placed obliquely to the graph)
  • live counter is a good option, I will implement it
  • smaller corner radius's good idea
  • Do you think that left axis need title?
  • with 4 hours of scope (5 min intervals) chart is not readable. In following scenario prob. line chart'd be better.
chart

I think grey background looks bad, we don't need left title. Maybe change intervals to 10 minutes or try with line chart.

@mikolaj-jalocha mikolaj-jalocha changed the title SKS chart SKS chart - WIP ⚒️ Dec 1, 2024
@mikolaj-jalocha mikolaj-jalocha marked this pull request as draft December 1, 2024 20:12
@24bartixx
Copy link
Member

Greate job!
I suggest adding orange opacity without a border to the prediction bars rather than outlined bars for better readability 😄

@mmzek
Copy link
Member

mmzek commented Dec 2, 2024

Hi, I'm not sure if I'm qualified to write this suggestion, but I thought it might be a good idea to include some information on the screen that high activity in SKS might be due to specific events. Perhaps we could add a message like “Find out more here” linking to resources such as the Student Council’s fb page or another platform where this type of information is shared (honestly, I have no idea where that might be).

@mikolaj-jalocha
Copy link
Member Author

Hi, I'm not sure if I'm qualified to write this suggestion, but I thought it might be a good idea to include some information on the screen that high activity in SKS might be due to specific events. Perhaps we could add a message like “Find out more here” linking to resources such as the Student Council’s fb page or another platform where this type of information is shared (honestly, I have no idea where that might be).

Everyone is qualified, each of us creates this app :-) Brilliant suggestion. I will create widget like that displaying when unusuall number of people's been measured

@simon-the-shark
Copy link
Member

Eventually the idea makes sense, but probably we could wait until we make the events/calendar feature that is planned next year.

@simon-the-shark
Copy link
Member

And detecting "unusual activity" seems like a backend task

@simon-the-shark
Copy link
Member

We can drop it to placeholder ideas for now?

@mikolaj-jalocha
Copy link
Member Author

mikolaj-jalocha commented Dec 3, 2024 •
edited
Loading

And detecting "unusual activity" seems like a backend task

Maybe, but in ultra easy version (that could be easily implemented at mobile side) it could be as simple as displaying widget when measured activity was higher that forecasted one

Copilot
Copilot AI reviewed Dec 11, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 14 out of 26 changed files in this pull request and generated no suggestions.

Files not reviewed (12)
  • lib/config/ui_config.dart: Language not supported
  • lib/features/navigator/app_router.dart: Language not supported
  • lib/features/parking_chart/widgets/chart_widget.dart: Language not supported
  • lib/features/sks_chart/data/models/sks_chart_data.dart: Language not supported
  • lib/features/sks_chart/data/repository/sks_chart_repository.dart: Language not supported
  • lib/features/sks_chart/presentation/chart_elements/sks_chart_bar_touch_data.dart: Language not supported
  • lib/features/sks_chart/presentation/chart_elements/sks_chart_border_data.dart: Language not supported
  • lib/features/sks_chart/presentation/chart_elements/sks_chart_grid_data.dart: Language not supported
  • lib/features/sks_chart/presentation/chart_elements/sks_chart_labels.dart: Language not supported
  • lib/features/sks_chart/presentation/chart_elements/sks_chart_legend_item.dart: Language not supported
  • lib/features/sks_chart/presentation/sks_chart.dart: Language not supported
  • lib/features/sks_chart/presentation/sks_chart_sheet.dart: Language not supported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@simon-the-shark simon-the-shark Awaiting requested review from simon-the-shark

@tomasz-trela tomasz-trela Awaiting requested review from tomasz-trela

At least 1 approving review is required to merge this pull request.

Assignees

@mikolaj-jalocha mikolaj-jalocha

Labels
enhancement New feature or request
Projects
ToPWR
Status: In review
Milestone
v1.2.0 - Mr. Shark, We Have to Cook
Development

Successfully merging this pull request may close these issues.
5 participants
@mikolaj-jalocha @simon-the-shark @tomasz-trela @24bartixx @mmzek