display the total daily gaps percantage #57

ArkadiK94 · 2023-09-20T03:26:25Z

Displaying the total daily gaps percentage in the "gaps" page

issue Display the total daily gaps percentage in the "gaps" page #14

NoamGaash

Thanks for your help!
here are a few comments,
I believe implementing those suggestions could increase code quality, but let me know what you're thinking

NoamGaash · 2023-09-22T21:07:32Z

src/pages/GapsPage.tsx

+  switch (true) {
+    case gapsPrecentage === 0:
+      return (
+        <TitleCellPrecentage style={{ color: '#097d09' }}>


colors can be moved to constants to improve readability and consistency

No problem, I can also add css classes for these colors, so we could use them in other elements if needed. Name of the classes: "great-result" (for green), "average-result" (for yellow), "terrible-result" (for red)

even better!

NoamGaash · 2023-09-22T21:11:17Z

src/pages/GapsPage.tsx

-        .then(setGaps)
+        .then((gaps) => {
+          setGaps(gaps)
+          const ridesInTime = gaps?.filter((gap) => formatStatus([], gap) === TEXTS.ride_as_planned)


Let's keep the formatting separated from logic. In the future, the formatting function may wrap the text and apply various modifications, so we probably don't want it to be coupled with this filter

NoamGaash · 2023-09-22T21:12:34Z

src/pages/GapsPage.tsx


 const GapsPage = () => {
  const { search, setSearch } = useContext(SearchContext)
  const { operatorId, lineNumber, timestamp, routes, routeKey } = search
  const [gaps, setGaps] = useState<GapsList>()
+  const [gapsPrecentage, setGapsPrecentage] = useState<number>()


please try to keep as few state variables as possible

src/pages/GapsPage.tsx

NoamGaash

Thanks for your help!
here are a few comments,
I believe implementing those suggestions could increase code quality, but let me know what you're thinking

ArkadiK94 · 2023-09-23T16:44:45Z

@NoamGaash I implemented the suggestions. Please, review them again.

NoamGaash

overall, it looks great,
but I think we should reconsider the hard-coded thresholds in you files
what do you think?

src/pages/components/DisplayGapsPercentage.tsx

NoamGaash · 2023-09-26T06:09:36Z

src/resources/variables.scss

+$great-result: #097d09;
+$average-result: #b4b407;
+$terrible-result: #b90a0a;


I love these namings!

NoamGaash · 2023-09-26T06:20:39Z

src/pages/components/DisplayGapsPercentage.tsx

+  let status = ''
+  switch (true) {
+    case gapsPrecentage === 0:
+      status = 'great'
+      break
+    case gapsPrecentage < 20:
+      status = 'average'
+      break
+    case gapsPrecentage >= 20:
+      status = 'terrible'
+      break
+  }


are you sure about the numbers?
if the gap percentage is 1/10,000, will you consider it "average"?
also, let's try something cleaner
for example -

const statuses = [ { min: -Infinity, status: "invalid" }, { min: 0, status: "great" }, { min: 10, status: "average" }, { min: 20, status: "terrible" }, { min: 101, status: "invalid" }, ] function getStatus(percentage) { return statuses.findLast(status => status.min < percentage).status }

This also relates to my coloring method in the new "gaps patterns page", and I think we should be consistent with our coloring \ terms. So maybe, even something like that could be better and used by both pages:

const statuses = [ { min: -Infinity, status: "invalid" }, { min: 0, status: "great", color: "green" // or a more specific color code }, { min: 10, status: "average", color: "orange" }, { min: 20, status: "terrible", color: "red" }, { min: 101, status: "invalid" }, ]

@ShayAdler I didn't understand to which page you are referring to because my changes are for the GapsPage as well.

The 'status' variable is for choosing one of these classes

&-great-result { color: $great-result; } &-average-result { color: $average-result; } &-terrible-result { color: $terrible-result; }

the variables (in scss file) contain

$great-result: #097d09; $average-result: #b4b407; $terrible-result: #b90a0a;

Did you mean to change the colors in the variables? as follows:

$great-result: green; $average-result: orange; $terrible-result: red;

@NoamGaash

if the gap percentage is 1/10,000, will you consider it "average"?

Yes, the green is for TEXTS.all_rides_completed
the average is for missing some rides
and the red is for missing more than 20% of rides.
Make sense and this is also the requirements from the issue

green - "כל הנסיעות בוצעו"
yellow - < 20% miss
red - >20%miss

It make sense because even if one bus will not make the ride as scheduled it can have some bad consequences on a group of people. They can late for work , for a meeting , take their child from school. So yeah, we count on the buses to take us to important places. When one bus will not make it on time we will need to wait another half an hour for the next bus (in some cities)

I prefer your cleaner code. Do you want me to make the boundaries configurable as well? I refer to the 10%,20% ?

I thought about this, and maybe the "average-result" keyword is confusing here. I think to change it to "decent-result". Hmm, what do you think?

agreed, you're right

ArkadiK94 · 2023-09-30T13:08:43Z

@NoamGaash @ShayAdler Hi, could you answer to my questions about the comments so I will make the right modifications?

NoamGaash

approved with comments
thank you for your help!!!

src/pages/GapsPage.tsx

NoamGaash · 2023-10-01T21:07:32Z

src/pages/components/DisplayGapsPercentage.tsx

+  let status = ''
+  switch (true) {
+    case gapsPrecentage === 0:
+      status = 'great'
+      break
+    case gapsPrecentage < 20:
+      status = 'average'
+      break
+    case gapsPrecentage >= 20:
+      status = 'terrible'
+      break
+  }


agreed, you're right

…component

ArkadiK94 · 2023-10-02T08:26:53Z

I changed the code. To use the findLast() method of an array I had to update the typescript version. @NoamGaash

NoamGaash · 2023-10-04T20:26:55Z

do you need help with the conflicts?

ArkadiK94 · 2023-10-04T21:19:34Z

There is some error, it is not related to my changes... Do you want me to open an issue about it?

NoamGaash · 2023-10-05T07:04:28Z

@ArkadiK94
try to run git pull and yarn

ArkadiK94 · 2023-10-05T19:37:36Z

Great, thanks , the error is not shown. Could you merge this pr?

NoamGaash · 2023-10-06T11:34:59Z

Sure! thank you!

aviv1620 · 2023-10-06T13:33:58Z

you change the typescript version in package.json but not in yarn.lock.
So every time I or someone runs yarn install the file changes.
Can you run yarn install and then commit push and merge again?

display the total daily gaps percantage

f692a31

NoamGaash requested a review from ShayAdler September 20, 2023 07:32

changing the colors of the text to darker colors

45d9d87

NoamGaash approved these changes Sep 22, 2023

View reviewed changes

NoamGaash requested changes Sep 22, 2023

View reviewed changes

making some changes as requested by the code reviewer

d554db8

ArkadiK94 requested a review from NoamGaash September 24, 2023 11:11

NoamGaash requested changes Sep 26, 2023

View reviewed changes

NoamGaash approved these changes Oct 1, 2023

View reviewed changes

modifing DisplayGapsPercentagecomponent for cleaner code and make it …

ac199ff

…component

NoamGaash enabled auto-merge (squash) October 4, 2023 20:25

NoamGaash disabled auto-merge October 4, 2023 20:26

NoamGaash approved these changes Oct 4, 2023

View reviewed changes

ArkadiK94 added 2 commits October 5, 2023 00:14

Resolve Conflicts

c1516c2

Merge branch 'ArkadiK94-usability/gaps' into usability/gaps

b7efbae

NoamGaash and others added 2 commits October 5, 2023 10:04

Merge branch 'main' into usability/gaps

893b336

Merge branch 'hasadna:main' into usability/gaps

933d905

NoamGaash merged commit 046ce74 into hasadna:main Oct 6, 2023
4 checks passed

NoamGaash mentioned this pull request Oct 6, 2023

chore: yarn lock and responsiveness #99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

display the total daily gaps percantage #57

display the total daily gaps percantage #57

ArkadiK94 commented Sep 20, 2023

NoamGaash left a comment

NoamGaash Sep 22, 2023

ArkadiK94 Sep 22, 2023

NoamGaash Sep 22, 2023

NoamGaash Sep 22, 2023

NoamGaash Sep 22, 2023

NoamGaash left a comment

ArkadiK94 commented Sep 23, 2023

NoamGaash left a comment

NoamGaash Sep 26, 2023

ArkadiK94 Sep 26, 2023

NoamGaash Sep 26, 2023

ShayAdler Sep 26, 2023

ArkadiK94 Sep 26, 2023 •

edited

Loading

ArkadiK94 Sep 26, 2023 •

edited

Loading

NoamGaash Oct 1, 2023

ArkadiK94 commented Sep 30, 2023

NoamGaash left a comment

NoamGaash Oct 1, 2023

ArkadiK94 commented Oct 2, 2023

NoamGaash commented Oct 4, 2023

ArkadiK94 commented Oct 4, 2023

NoamGaash commented Oct 5, 2023 •

edited

Loading

ArkadiK94 commented Oct 5, 2023

NoamGaash commented Oct 6, 2023

aviv1620 commented Oct 6, 2023 •

edited

Loading

display the total daily gaps percantage #57

display the total daily gaps percantage #57

Conversation

ArkadiK94 commented Sep 20, 2023

NoamGaash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoamGaash left a comment

Choose a reason for hiding this comment

ArkadiK94 commented Sep 23, 2023

NoamGaash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArkadiK94 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

ArkadiK94 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArkadiK94 commented Sep 30, 2023

NoamGaash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArkadiK94 commented Oct 2, 2023

NoamGaash commented Oct 4, 2023

ArkadiK94 commented Oct 4, 2023

NoamGaash commented Oct 5, 2023 • edited Loading

ArkadiK94 commented Oct 5, 2023

NoamGaash commented Oct 6, 2023

aviv1620 commented Oct 6, 2023 • edited Loading

ArkadiK94 Sep 26, 2023 •

edited

Loading

ArkadiK94 Sep 26, 2023 •

edited

Loading

NoamGaash commented Oct 5, 2023 •

edited

Loading

aviv1620 commented Oct 6, 2023 •

edited

Loading