UI components #11
UI components #11
Conversation
There was a problem hiding this comment.
@tpardeshi awesome work on getting the front end together, there are small changes need to be done before we can merge this PR.
@pravins I and @tpardeshi have discussed the future changes in which we will make the code more restructured and readable, also we would be adding unit test cases as we start improving the tool further.
src/App.js
Outdated
| </div> | ||
| <BrowserRouter> | ||
| <Switch> | ||
| <> |
There was a problem hiding this comment.
We can remove React Fragments from here as we have the wrapper.
src/Components/SimpleForm.js
Outdated
| const [selectProductsVersion, setSelectProductsVersion] = useState(""); | ||
| const [selectLocales, setSelectLocales] = useState(""); | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
We can remove React Fragments from here as well.
Also, add a new line at the end of every file.
src/Components/SimpleForm.js
Outdated
| </FormSelect> | ||
| </FormGroup> | ||
| <ActionGroup> | ||
| <Button type="submit" value="Submit">Submit</Button> |
There was a problem hiding this comment.
Remove the value from here, it is not required as we are using Patternfly components.
If it had been an HTML input button then we could use the value attribute.
src/Screens/Products.js
Outdated
| </TextContent> | ||
| </Link> | ||
| </CardBody> | ||
| <CardFooter></CardFooter> |
tpardeshi
force-pushed
the
ui-components
branch
from
fac820e to
c88f795
Compare
|
@akkias Thanks for the discussions with Twinkle. I agree with both of you. |
tpardeshi
force-pushed
the
ui-components
branch
from
c88f795 to
71d0364
Compare
| @@ -1,17 +1,40 @@ | |||
| { | |||
| "name": "imagematch", | |||
| "name": "frontend-demo", | |||
There was a problem hiding this comment.
This must name of the project and not the purpose of a demonstration.
| "@storybook/addons": "^5.3.8", | ||
| "@storybook/preset-create-react-app": "^1.5.2", | ||
| "@storybook/react": "^5.3.8", | ||
| "eslint-plugin-react-hooks": "^4.0.8" |
There was a problem hiding this comment.
Are we making of the eslint, because I see many syntax error that lint would not allow before merging.
| "react": "^16.10.1", | ||
| "react-dom": "^16.10.1", | ||
| "react-scripts": "3.1.2" | ||
| "@patternfly/react-core": "^3.129.3", |
There was a problem hiding this comment.
Are we not using pattern fly 4? pf3 has flaws and all projects that are being developed at current time have moved to pf4. Its better you do all your development in pf4.
| const logoProps = { | ||
| href: "https://github.com/lingostack", | ||
| target: "_blank", | ||
| }; | ||
| return ( |
There was a problem hiding this comment.
There is no prop validation for this?
| <nav className="pf-c-breadcrumb" aria-label="breadcrumb"> | ||
| <ol className="pf-c-breadcrumb__list"> | ||
| <li className="pf-c-breadcrumb__item"> | ||
| <Link to="/" className="pf-c-breadcrumb__link">Products</Link> | ||
| <span className="pf-c-breadcrumb__item-divider"> | ||
| <AngleRightIcon /> | ||
| </span> | ||
| </li> | ||
| <li className="pf-c-breadcrumb__item"> | ||
| <span className="pf-c-breadcrumb__link pf-m-current" aria-current="page"> | ||
| Versions | ||
| </span> | ||
| </li> | ||
| </ol> | ||
| </nav> |
There was a problem hiding this comment.
This is ugly, in pf4 you can do something like:
import React from 'react';
import { Breadcrumb, BreadcrumbItem, BreadcrumbHeading } from '@patternfly/react-core';
SimpleBreadcrumbs = () => (
<Breadcrumb>
<BreadcrumbItem to="#">Section home</BreadcrumbItem>
<BreadcrumbItem to="#">Section title</BreadcrumbItem>
<BreadcrumbItem to="#">Section title</BreadcrumbItem>
<BreadcrumbItem to="#" isActive>
Section Landing
</BreadcrumbItem>
</Breadcrumb>
);
There was a problem hiding this comment.
@rabajaj0509 Yes, we can do this now.
When we started working on this app at that time the patternfly didn't implement the react-router with the breadcrumb and used product anchor tag with it and that was the reason we had to hardcode the breadcrumb component.
| <CardBody> | ||
| <SimpleEmptyState /> | ||
| <Form onSubmit={props.handleSubmit}> |
There was a problem hiding this comment.
What might the reason to use a emptystate component inside a card?
There was a problem hiding this comment.
If someone uses the direct URL to land on a certain page then we show the message that you need to select options from the form to check the diff.
Using query params is a part of the next PR and we have already discussed that and conveyed it to @pravins
| const [currentPage, setCurrentPage] = useState(0); | ||
| const [page, setPage] = useState(1); | ||
| const [perPage, setPerPage] = useState(10); | ||
| const [productsVersion, setProductsVersion] = useState([]); |
There was a problem hiding this comment.
You have set this multiple times? is that required?
There was a problem hiding this comment.
Yes, that's how Patternfly implemented the pagination, but if you have any better way then, please suggest.
There was a problem hiding this comment.
I am so confused looking at this PR for the following reasons:
- It has so many components in just 1 PR.
- Have you tried to run lint on this codebase? There will be so many errors that have gone unnoticed here.
- There is no prop validation at all.
- The package.json has many dependencies which I believe are not required.
Few suggestions:
- Take packaging seriously, only use modules that are required
- Setup a github action, for running lint on your PRs.
- Create small PRs
- Attach screen shots to your PR else how will the reviewer understand what you are tryign to do.
- Use PF4! please dont use pf3 now.
These are few things that come to my mind when I see this PR. I am not a react expert and have not tested the PR.
|
@rabajaj0509 Thanks for the review. We will surely consider all this suggestions in upcoming improvement pull requests. As of now I think lets have a base ready for the project, we will continue with the improvements further. |
|
Sure, go ahead! |
Developed UI components for all the APIs as per backend