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

pull request #4

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

pull request #4

wants to merge 54 commits into from

Conversation

SamrithaS
Copy link
Collaborator

react boiler plate and tooling is done

react boiler plate and tooling is done
App.js Outdated
>
<MenuGroup>
<Flex>
<EachMenuItem

Choose a reason for hiding this comment

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

Suggested change
<EachMenuItem
<DropDown

Choose a reason for hiding this comment

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

Since Dropdown is the actual component

{props.leftIcon?(<Box as={props.leftIcon} pb={1} />):(<> </>)}
<MenuButton border="none" pr={8} fontSize={14}>
{props.menuItemName}
{ props.TagNumber>0?(<Tag

Choose a reason for hiding this comment

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

Linting issue

import indexOf from "lodash/indexOf";
import remove from "lodash/remove";
function DropDown(props) {
const DropDownArray = ["Apartment", "Corporate", "Floorplan", "Photo"];

Choose a reason for hiding this comment

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

Suggested change
const DropDownArray = ["Apartment", "Corporate", "Floorplan", "Photo"];
const dropDownArray = ["Apartment", "Corporate", "Floorplan", "Photo"];

function DropDown(props) {
const DropDownArray = ["Apartment", "Corporate", "Floorplan", "Photo"];
const [selectedArray, setSelectedArray] = useState([]);
const CheckboxClicked = selectedItem => {

Choose a reason for hiding this comment

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

Suggested change
const CheckboxClicked = selectedItem => {
const checkboxClicked = selectedItem => {

App.js Outdated
import DropDown from "./components/header.js";
function App() {
const [offsetValue, setOffsetValue] = useState(0);
const [limitValue, setLimitValue] = useState(9);

Choose a reason for hiding this comment

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

Replace with use reducer.

App.js Outdated
const [pageNumber, setPageNumber] = useState(1);
const paginationButtonClick = numberClicked => {
setOffsetValue(numberClicked * 9);
setLimitValue((numberClicked + 1) * 9);

Choose a reason for hiding this comment

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

This pattern of updating three useState effects for one single Callback will trigger 3 different renders. Use useReducer instead.

import { Image, Heading, Stack, Text, Box } from "@chakra-ui/core";
import map from "lodash/map";
import { TiLocationOutline } from "react-icons/ti";
import Image1 from "../Images/Image1.jpeg";

Choose a reason for hiding this comment

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

Pagination component shouldn't hold any data. It should get through props.

Copy link

@prasanna1211 prasanna1211 left a comment

Choose a reason for hiding this comment

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

totalCount: Total number of items that is passed (edited)
limit: limit for each page (edited)
maxStartRangeCount: how many to show at start (edited)
maxEndRangeCount: how many to show at end (edited)
initialStartPage (in this example should be between 0 - 13)

onPageClick based on this callback you can render new set of images

labelEdit would be a function which takes in the page number and whatever user returns will be the custom page number. For example if user wants 'A' 'B' 'C' ... this function will help in that.

for example if there are 125 total count and limit is 10 it will have 13 pages
if maxStartRangeCount is 3 and maxEndRangeCount it will show [1][2][3] .. CurrentPage ... [11][12][13]

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.

2 participants