component for recognition listing #163
Conversation
avinash-mane
force-pushed
the
feature/react/159/core-component-and-shared-component-for-recognition-listing
branch
from
c7bb69d to
eaad3ee
Compare
avinash-mane
changed the title
| }) => { | ||
| return ( | ||
| <Row> | ||
| <Row className="d-flex justify-content-end"> |
react-frontend/src/shared-components/core-value-icon-components/CoreValueIconComponent.js
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,11 @@ | |||
| import React from "react"; | |||
| import { render } from "@testing-library/react"; | |||
| import CoreValueIconComponent from "./CoreValueIconComponent"; | |||
| `; | ||
|
|
||
| const RecognitionTextComponent = ({ text, given_by }) => ( | ||
| <Row className="d-flex flex-column"> |
There was a problem hiding this comment.
do we need d-flex class also and Row also?
| core_value, | ||
| }) => { | ||
| return ( | ||
| <Row> |
There was a problem hiding this comment.
I am not specifying the width this row occupies in container, neither I am using the col under it, do we need these?
| </Img> | ||
| <Row className="d-flex flex-column text-start ml-4"> | ||
| <Form.Label className="font-weight-bold ">{given_for}</Form.Label> | ||
| <Row className=" d-sm-block d-xs-block d-md-none"> |
| }; | ||
|
|
||
| RecognitionListComponent.propTypes = { | ||
| recognitionList: PropTypes.array.isRequired, |
There was a problem hiding this comment.
why the inconsistent naming convention? some places you have maintained kebab case for props/variables and some places camelCase? use camelCase always for props/variables
cc: @sahilbhatia
| overflow: hidden; | ||
| `; | ||
|
|
||
| const ImgM = styled.div` |
| <ImgM className="bg-dark d-sm-block d-md-none mt-2"> | ||
| <ImageComponent | ||
| src="https://i.picsum.photos/id/654/300/200.jpg" | ||
| alt="CoreValueImage" |
There was a problem hiding this comment.
what is the purpose of alt text? I believe it should be user-readable, then why TitleCase here?
| <RecognitionTextComponent text={text} given_by={given_by} /> | ||
| </Col> | ||
| <Col className="d-none d-md-block"> | ||
| <Col className=" d-flex justify-content-center"> |
There was a problem hiding this comment.
read about how to have nested Grid
| import RecognitionTextComponent from "recognition-list-components/RecognitionTextComponent"; | ||
| import HighFiveComponent from "shared-components/high-five-components/HighFiveComponent"; | ||
|
|
||
| const giveHighFive = () => { |
There was a problem hiding this comment.
Why do we need an empty function?
| //to to for give high five | ||
| }; | ||
|
|
||
| const ImgD = styled.div` |
There was a problem hiding this comment.
What does imgD stand for?
| const ImgM = styled.div` | ||
| border-radius: 20px; | ||
| height: auto; | ||
| width: auto; |
There was a problem hiding this comment.
-
Given that both height and width are auto, do we need to specify them?
-
Why overflow has been set to auto?
| }) => ( | ||
| <Card | ||
| className="my-4 mx-2 shadow p-3 mb-4 border border-secondary bg-light grey" | ||
| style={{ borderRadius: "36px" }} |
There was a problem hiding this comment.
Do not use inline styles. We are already using styled components
| /> | ||
| <ImgM className="bg-dark d-sm-block d-md-none mt-2"> | ||
| <ImageComponent | ||
| src="https://i.picsum.photos/id/654/300/200.jpg" |
There was a problem hiding this comment.
Why is this value hard coded inside component definition?
| </ImgM> | ||
| <RecognitionTextComponent text={text} given_by={given_by} /> | ||
| </Col> | ||
| <Col className="d-none d-md-block"> |
There was a problem hiding this comment.
Read and understand bootstrap grid:
avinash-mane
force-pushed
the
feature/react/159/core-component-and-shared-component-for-recognition-listing
branch
from
48ba991 to
ff4b0e7
Compare
No description provided.