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

Gaps patterns page #44

Merged
merged 26 commits into from
Oct 10, 2023
Merged

Gaps patterns page #44

merged 26 commits into from
Oct 10, 2023

Conversation

ShayAdler
Copy link
Collaborator

@ShayAdler ShayAdler commented Sep 16, 2023 •
edited
Loading

image

Issue #15

NoamGaash
NoamGaash approved these changes Sep 17, 2023
View reviewed changes
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.

some comments :)
great work!!
it's so great to see you here on the frontend side 🥳

src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
@NoamGaash NoamGaash self-requested a review September 17, 2023 20:32
@ShayAdler ShayAdler marked this pull request as ready for review September 24, 2023 11:36
ShayAdler
ShayAdler commented Sep 24, 2023
View reviewed changes
Copy link
Collaborator Author

@ShayAdler ShayAdler left a comment

Choose a reason for hiding this comment

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

Thanks for the review!
Still struggling with the test (because of the dates), so I'll add them in a separate PR :)

NoamGaash
NoamGaash requested changes Sep 26, 2023
View reviewed changes
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.

I'll fix those comments, enjoy your vocation! 🥳

src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
src/pages/components/DateTimePicker.tsx Outdated Show resolved Hide resolved
src/pages/components/DateTimePicker.tsx Show resolved Hide resolved
src/pages/GapsPatternsPage.tsx Outdated Show resolved Hide resolved
@shootermv shootermv force-pushed the gaps-patterns-page branch 2 times, most recently from d434b02 to a14b113 Compare October 9, 2023 04:06
AdlerShay and others added 21 commits October 9, 2023 11:41
@AdlerShay @shootermv
to & from dates in gap service
5b88e6b
@AdlerShay @shootermv
gaps patterns page
3441ab8
@AdlerShay @shootermv
fix: same scaling to both axes
5e3b104
@AdlerShay @shootermv
order by support
e110d71
@AdlerShay @shootermv
custom tooltip
796265f
@AdlerShay @shootermv
severity sorting fix, Hebrew text
bdf12be
@ShayAdler @shootermv
remove actualPercentage floating point
0711764
@ShayAdler @NoamGaash @shootermv
Update src/pages/GapsPatternsPage.tsx
61ba372 
Co-authored-by: NoamGaash <[email protected]>
@ShayAdler @shootermv
fix: cr
6c39e7e
@ShayAdler @shootermv
fix: sorting useEffect
c733b50
@ShayAdler @shootermv
fix: cr
a1a1608
@shootermv
move style to scss file
d704908
@shootermv @NoamGaash
remove BarChart ref
37c2337 
Co-authored-by: NoamGaash <[email protected]>
@shootermv @NoamGaash
remove Line ref
7ed8c82 
Co-authored-by: NoamGaash <[email protected]>
@shootermv @NoamGaash
remove extra label param
09381d7 
Co-authored-by: NoamGaash <[email protected]>
@shootermv
remove extra breakline
190dcd8
@shootermv @NoamGaash
Usedate -> useDate
714ab15 
Co-authored-by: NoamGaash <[email protected]>
@shootermv
move bySeverity to utils
d0458d5
@shootermv
move byHour handler to utils
18bfaa3
@shootermv
move colorByExecution handler to utils
d1e5eaf
@shootermv
move logic to useGapsLsit hook
fd277c6
NoamGaash
NoamGaash reviewed Oct 9, 2023
View reviewed changes
src/pages/TimelinePage.tsx
@@ -62,7 +62,7 @@ const TimelinePage = () => {
return
}
setRoutesIsLoading(true)
getRoutesAsync(moment(timestamp), operatorId, lineNumber)
getRoutesAsync(moment(timestamp), moment(timestamp), operatorId, lineNumber)
Copy link
Member

Choose a reason for hiding this comment

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

on purpose?

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

getRoutesAnsync method has toTimestamp param

NoamGaash
NoamGaash approved these changes Oct 10, 2023
View reviewed changes
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.

@ShayAdler @shootermv thank you both!
the code looks great, and I trust you to verify the UI behaviour.
feel free to merge it once you feel it's ready ⭐

@shootermv
Copy link
Collaborator

i would be glad if you can test the functionality too...
currently im not trust myself anough

@shootermv shootermv force-pushed the gaps-patterns-page branch 2 times, most recently from 9d6db5f to c21655f Compare October 10, 2023 11:55
@shootermv shootermv merged commit f386f62 into hasadna:main Oct 10, 2023
4 checks passed
@ArkadiK94 ArkadiK94 mentioned this pull request Oct 21, 2023
Closed
@github-actions github-actions bot mentioned this pull request Dec 18, 2023
Merged
@amabelleS amabelleS mentioned this pull request Feb 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NoamGaash NoamGaash NoamGaash approved these changes

@shootermv shootermv shootermv left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShayAdler @shootermv @NoamGaash @AdlerShay