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

fix: fix map by line time related bugs #936

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TomRytt
Copy link
Collaborator

@TomRytt TomRytt commented Nov 20, 2024

Description

Fixes #902.

Fixes the following issues:

  1. Single lines times stopped at 6PM
  2. First lines were from 11PM the day before
  3. The wrong time was used to filter the times (recorded at times instead of scheduled start time) - which resulted in issue 2.

Optimizations:

  1. Optimized the useMemo to return an empty array if no positions passed the filter (typically when no line is chosen yet)
  2. Switched to using a Set to ensure times can only appear in options once

screenshots

image

Taken at 22:58 (two minutes before the next bus is scheduled)
image

Taken at 23:00 (after the bus is scheduled)
image

@TomRytt TomRytt changed the title Fix map by line time related bugs Fix: map by line time related bugs Nov 20, 2024
@TomRytt TomRytt changed the title Fix: map by line time related bugs fix: fix map by line time related bugs Nov 20, 2024
Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

Great, thank you very much.
Consider adding tests or adding an issue describing the test cases that would cover this bug to prevent future regressions

@@ -35,17 +37,45 @@ export const useSingleLineData = (lineRef?: number, routeIds?: number[]) => {
return pos
}, [locations])

function convertTo24HourAndToNumber(time: string): number {
Copy link
Member

Choose a reason for hiding this comment

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

The name does not reflect that the output of this method is the number of minutes since midnight. Would you consider renaming this method?
This is my suggestion, but feel free to implement anything you like.

Suggested change
function convertTo24HourAndToNumber(time: string): number {
function timestringToMinutes(time: string): number {

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.

fix: מפה לפי קו - לא מוצגות נסיעות לאחר השעה 6 בערב
2 participants