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

Better inputs design fix placement #61

Merged
merged 22 commits into from
Oct 1, 2023

Conversation

aviv1620
Copy link
Collaborator

comment on issue #39

@aviv1620
Copy link
Collaborator Author

The Playwright Tests run when the server is busy.
if you run the test again(without change) it works.
recommend increasing the timeout.

Copy link
Collaborator

@ArkadiK94 ArkadiK94 left a comment

Choose a reason for hiding this comment

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

The layout of the input fields looks great now, thanks. I have only some minor comments for you.

Copy link
Collaborator

@ArkadiK94 ArkadiK94 left a comment

Choose a reason for hiding this comment

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

The comments :)

src/pages/components/GridSelectorAndLabel.tsx Outdated Show resolved Hide resolved
src/pages/RealtimeMapPage.tsx Show resolved Hide resolved
src/pages/RealtimeMapPage.tsx Show resolved Hide resolved
src/pages/components/DataAndTimeSelector.tsx Outdated Show resolved Hide resolved
src/pages/components/DataAndTimeSelector.tsx Outdated Show resolved Hide resolved
src/pages/components/Label.tsx Outdated Show resolved Hide resolved
src/pages/components/StopSelector.tsx Outdated Show resolved Hide resolved
@ArkadiK94 ArkadiK94 mentioned this pull request Sep 22, 2023
@aviv1620
Copy link
Collaborator Author

OK.
On Tuesday I upstream from main(to prevent conflicts) and then improve the code by the comments.

@ArkadiK94 ArkadiK94 self-requested a review September 27, 2023 07:03
@aviv1620
Copy link
Collaborator Author

aviv1620 commented Sep 27, 2023

fix the comments.
Every page works(test manual).
The PR ready to review.

for another PR:
Playwright has a problem with the har file. If I route the traffic to network the test pass.

Copy link
Collaborator

@ArkadiK94 ArkadiK94 left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, I have only some comments.

const value = ts ? ts.format() : ''
setFrom(value)
setTo(formatTime(+new Date(value) + (+new Date(to) - +new Date(from)))) // keep the same time difference
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should declare the function that you use in setTimeValid before returning the JSX (rendering the content) and use it here and in setTimeValid of TimeSelector so you don't duplicate code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the state form and the state to design before Noam adds the load date from the URL feature.
I prefer to remove the state to completely.
add the minutes when we call useVehicleLocations.
and use the moment.js library.

this refactor related to issue #79 and issue #71.
so I prefer to solve this in another PR.

src/pages/components/DataSelector.tsx Outdated Show resolved Hide resolved
@ArkadiK94
Copy link
Collaborator

@NoamGaash I reviewed the code , would you like to review it as well?

@NoamGaash
Copy link
Member

NoamGaash commented Oct 1, 2023

@ArkadiK94 @aviv1620 thank you!
Let's merge it 😄

@NoamGaash NoamGaash merged commit 45584f2 into hasadna:main Oct 1, 2023
4 checks passed
@NoamGaash
Copy link
Member

@all-contributors
please add @aviv1620 for code
please add @ArkadiK94 for review

Copy link
Contributor

@NoamGaash

I've updated the pull request to add @aviv1620! 🎉

I've put up a pull request to add @ArkadiK94! 🎉

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