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

feat(bus): option to mark bus as delayed #1538

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

IshanA2007
Copy link

Closes #1414

Proposed changes

  • Added the option to mark a bus as delayed in the afternoon bus page

Brief description of rationale

-So people can see if their bus is delayed.

@IshanA2007 IshanA2007 requested a review from a team as a code owner May 6, 2023 23:51
@IshanA2007 IshanA2007 changed the title feat(markbusdelayed)option to mark bus as delayed feat(markbusdelayed): option to mark bus as delayed May 7, 2023
@IshanA2007 IshanA2007 changed the title feat(markbusdelayed): option to mark bus as delayed feat(bus): option to mark bus as delayed May 7, 2023
Copy link
Member

@alanzhu0 alanzhu0 left a comment

Choose a reason for hiding this comment

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

Good work overall. Please see comments embedded in the code. A few overall points:

  • The only dropdown option for Fairfax buses (JT-###) should be to mark them as delayed. All other options (on time and arrived) should be removed - Fairfax buses are managed by assigning them to a position on the bus map. Buses from other counties should have all 3 options (on time, arrived and delayed).
  • When a Fairfax bus is marked arrived on the map, the delayed option for that bus should be removed from the dropdown. It should be accessible only after unassigning the bus from its spot.
  • Marking a bus on time does not work. There appears to be a bug in your code for handling marking a bus on time.

intranet/static/js/bus-afternoon.js Outdated Show resolved Hide resolved
intranet/static/js/bus-afternoon.js Outdated Show resolved Hide resolved
intranet/static/js/bus-afternoon.js Outdated Show resolved Hide resolved
intranet/static/js/bus-afternoon.js Outdated Show resolved Hide resolved
intranet/static/js/bus-afternoon.js Outdated Show resolved Hide resolved
intranet/static/js/bus-afternoon.js Outdated Show resolved Hide resolved
@IshanA2007
Copy link
Author

Thanks for the feedback! I believe I fixed everything @alanzhu0

@coveralls
Copy link

Coverage Status

Coverage: 81.279%. Remained the same when pulling 81b26ac on IshanA2007:master into 7cc4dcb on tjcsl:dev.

Copy link
Contributor

@shahsalonik shahsalonik left a comment

Choose a reason for hiding this comment

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

LGTM! (@alanzhu0 feel free to double-check) Could you squash your commits? Thanks!

@IshanA2007 IshanA2007 force-pushed the master branch 4 times, most recently from 45142c5 to 8a17b16 Compare May 14, 2023 15:24
@IshanA2007
Copy link
Author

Commits successfully squashed!

@alanzhu0
Copy link
Member

I have a few comments - I'll add them later this evening after I get a chance to test a bit more.

@NotFish232
Copy link
Member

Make sure to remove all the artifacts from squashing your commits. I noticed a lot of what looks like merge conflict errors, i.e. >>>>>>> 45142c57 (feat(bus): option to mark bus as delayed) on line 218.

@alanzhu0
Copy link
Member

alanzhu0 commented May 16, 2023

Good work fixing the issues from before. Here are a few more comments. Note that none of these are actually bugs in your code - these are just UX subtleties that I think should be present. I've commented the ones I've noticed, but when you go about revising, think about what you would consider the smoothest UX and adjust the functionality accordingly.

  • After a Fairfax bus has been marked as delayed, there should be an option in the dropdown to mark it as on time again.
  • After a non-Fairfax bus has been marked as delayed, there should be an option in the dropdown to mark it as arrived, rather than having to mark it as on time and then arrived.

Alternatively, instead of dealing with these dropdown UX issues, you could redesign the UI to be smoother and directly improve the UX. For example, as I mentioned in the issue description, take a look at the morning bus page dropdown:
image
You could implement this for the afternoon bus page - much of it would be the same as the morning. The only difference would be for Fairfax buses, where the "Arrived" button should be grayed out so that it can be assigned a space on the map. Also look at the morning bus list:
image
It would also be useful to have this on the side of the bus page for bus admins to quickly check the status of all the buses (it can be annoying to look at the map for administrative functions). Again, this will likely amount to just a copy-paste from the morning bus page.

This UI improvement is optional. If you choose to do it, it would be the facelift the bus app has needed for a while, but if not, adding the delayed feature would be an improvement satisfying the issue.

Good luck!

@IshanA2007
Copy link
Author

I'll work on it!

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.

None yet

5 participants