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 realtime map issues #144

Merged
merged 18 commits into from
Oct 27, 2023
Merged

Conversation

amabelleS
Copy link
Collaborator

@amabelleS amabelleS commented Oct 22, 2023

Description

  • Changed:
const now = moment() (value for "to" in the state)
const oneMinuteAgo = moment().subtract(1, 'minutes') (value for "for")
// const fiveMinutesAgo = moment().subtract(5, 'minutes')
// const fourMinutesAgo = fiveMinutesAgo.add(1, 'minutes')
  • Set TimeSelector, onChange =>
  setFrom(moment(val).subtract(moment(to).diff(moment(from))))
  setTo(moment(val)) // keep the same time difference

*Same as DateSelector

  • MinuteSelector => setFrom to subtract from to the minutes difference

screenshots

image

@amabelleS amabelleS requested a review from NoamGaash as a code owner October 22, 2023 14:12
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 can see that you made several changes, but that's not a test,
Have you seen the tutorial I made?
I'm here for any question
https://www.youtube.com/watch?v=jEWG7nYKk_0

Comment on lines 49 to 50
const now = moment()
const oneMinuteAgo = moment().subtract(1, 'minutes')
Copy link
Member

Choose a reason for hiding this comment

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

We should now use now, because the API can't guarantee this information will be available. The ETL process may take some time

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should now use now, because the API can't guarantee this information will be available. The ETL process may take some time

Can we do that?:

const now = new Date()
const oneMinuteAgo = moment().subtract(1, 'minutes')
const [from, setFrom] = useState(oneMinuteAgo)
const [to, setTo] = useState(moment(now))

Copy link
Member

Choose a reason for hiding this comment

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

My bad - I wanted to say we should not use now.
If you'll try to load data that hasn't been processed (yet), the backend will return an empty array and the frontend code will memorize it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad - I wanted to say we should not use now. If you'll try to load data that hasn't been processed (yet), the backend will return an empty array and the frontend code will memorize it

I'm trying to understand the issue. I read it the first time "not" actually:) We should not use now:
I thought you meant the moment api, but now I understand that now as a variable is problematic.

So, as a default, we want the application first render to be four minutes from now (=to in the state), and five minutes ago (= from).

Copy link
Member

Choose a reason for hiding this comment

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

exactly

src/pages/RealtimeMapPage.tsx Show resolved Hide resolved
@amabelleS
Copy link
Collaborator Author

I can see that you made several changes, but that's not a test, Have you seen the tutorial I made? I'm here for any question https://www.youtube.com/watch?v=jEWG7nYKk_0

Honestly, I was confused about whether you wanted to add some tests, or maybe you meant QA and fix bugs, because there were...
According to the issue:

  1. When the page gets loaded, the difference between the start date and the end date should be one minute =>
    It was not. When I console.loge fourMinutesAgo & fiveMinutesAgo both show the time 4 minutes ago.
  2. When "from" date changes, the "to" date should change accordingly =>
    They were not, you could see that without the tests. The way I understand it, when the user renders the realTimeMap for the first time, he wants to see the date and time now, not 3 minutes ago. When he sets the time to another date/hour, he'll expect to see the time with the same minute difference that is set, whether the default 1, or whatever he sets. (& that covers the last part of the task to haha)

So, I can add tests, no problem. But first, we need a code that will make them pass:)

@NoamGaash NoamGaash changed the title Test realtime map issue118 fix realtime map issues Oct 24, 2023
@NoamGaash
Copy link
Member

sure! fixing bugs is great.
tests can be done later, on a different PR

@amabelleS
Copy link
Collaborator Author

sure! fixing bugs is great. tests can be done later, on a different PR

I would love to do the tests also, another PR sounds great:)

@amabelleS
Copy link
Collaborator Author

there are some Playwright tests that my PR did not pass. I'm trying to figure out now why:)

@shootermv
Copy link
Contributor

shootermv commented Oct 26, 2023

it is missing key of locales
you can rebase now and tests will hopefully pass

@NoamGaash NoamGaash changed the base branch from main to NoamGaash-patch-2 October 27, 2023 06:56
@NoamGaash NoamGaash changed the base branch from NoamGaash-patch-2 to main October 27, 2023 06:56
@netlify
Copy link

netlify bot commented Oct 27, 2023

Deploy Preview for astonishing-pothos-5f81bd ready!

Name Link
🔨 Latest commit 81f08be
🔍 Latest deploy log https://app.netlify.com/sites/astonishing-pothos-5f81bd/deploys/653b600af86d7000080332df
😎 Deploy Preview https://deploy-preview-144--astonishing-pothos-5f81bd.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

code looks great, and the functionality seems perfect!
Thank you! That's a great contribution 💪

@NoamGaash NoamGaash merged commit 6b69fa7 into hasadna:main Oct 27, 2023
9 checks passed
@amabelleS
Copy link
Collaborator Author

code looks great, and the functionality seems perfect! Thank you! That's a great contribution 💪

Thank you ❤️‍🔥 What a great project to contribute to 💯
It helped me a lot to be a part of it, a way to escape from all the chaos around here...

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.

3 participants