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

Add New Drive View #584

Merged

Conversation

vlakyi
Copy link
Contributor

@vlakyi vlakyi commented Apr 28, 2021

Add New Drive View. (#583)

arturtamborski and others added 7 commits January 7, 2021 12:47
…ForPoznan#556)

* Add CarCountryFilter for more advanced filtering in admin panel

* Enable CarCountryFilter on Refuel admin view

* Add Car Country column in Refuel admin view
* PAH-557 add Kurdish translation

* add kurdistan flag

* fix linter

* replace corrupted and missing translations to english, add iraq flag
@vlakyi vlakyi changed the title Pah add new drive form. Add New Drive View Apr 28, 2021
@arturtamborski arturtamborski linked an issue May 2, 2021 that may be closed by this pull request
Copy link
Member

@arturtamborski arturtamborski left a comment

Choose a reason for hiding this comment

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

Looks good but I have few comments.

@@ -0,0 +1,2 @@
# Convert to LF line endings on checkout.
*.sh text eol=lf
Copy link
Member

Choose a reason for hiding this comment

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

👍

{
path: '/drive',
key: routeKeys.DRIVE,
component: lazy(() => import('./views/Add Drive'))
Copy link
Member

Choose a reason for hiding this comment

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

space? I don't think that's a good idea

Suggested change
component: lazy(() => import('./views/Add Drive'))
component: lazy(() => import('./views/AddDrive'))

@@ -0,0 +1,208 @@
import React, { useState, useEffect } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

as above, please rename it to AddDrive


const validationSchema = yup.object().shape({
startLocation: yup.string().required(useT('Start location is required')),
mileageStart: yup.string().required(useT('Starting mileage is required')),
Copy link
Member

Choose a reason for hiding this comment

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

why string? this doesn't align with initialValues

})}
</Box>
<p>
{traveled} km {useT('traveled')}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{traveled} km {useT('traveled')}
{traveled} km {traveledT}

please keep the useT out of returned JSX template, it's easier to find them when they're at the beginning of the function rather than somewhere in the returned template.

…ged validation types in yup to number for mileage start and end, extracted traveledT variable otside JSX.)
@vlakyi
Copy link
Contributor Author

vlakyi commented May 2, 2021

Fixed issues mentioned above in the latest commit :)

{
path: '/drive',
key: routeKeys.DRIVE,
component: lazy(() => import('./views/AddDrive'))
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for pointing it out only now, but there's one inconsistency - this view is mapped under /drive, key is named DRIVE and the class is named DriveView, but the directory is named AddDrive. Can you rename it again to Drive?

I'm sorry for pointing it out this late but I just noticed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, I also was thinking about this inconsistency. Thank you for pointing this.

frontend-react/src/views/AddDrive/index.js Outdated Show resolved Hide resolved
frontend-react/src/views/AddDrive/index.js Outdated Show resolved Hide resolved
@arturtamborski arturtamborski merged commit adf9190 into CodeForPoznan:react-revolution May 2, 2021
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.

Add New Drive View
3 participants