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

Bhosseini/orders page #56

Merged
merged 64 commits into from
May 6, 2023
Merged

Bhosseini/orders page #56

merged 64 commits into from
May 6, 2023

Conversation

bahar-hosseini
Copy link
Collaborator

Create import CSV for admin Orders page (#20 )

Description

Task :
Create a file upload that reads in a CSV file with client data that renders a table with all read client info as seen in screenshots below.

Acceptance Criteria:
Information is displayed accurately
Search bar filters items in the table when a name is inputted
Renders a set amount of rows, some sort of button on the bottom to navigate the table

List any dependencies that are required for this change. (gems, js libraries, etc.)

  • SheetJS
  • @mui/x-data-grid

Warnings:

  • The only warning we are receiving is due to the way in which I used getRowId row identifier. I'm going to fix this after adding backend.
  • All helper functions and types are in Orders.tsx file. We have to create some directories like utils and interfaces/types to move them there.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

This feature has been tested by this Excel file.



//! Dynamic Header
// const range = XLSX.utils.decode_range(ws["!ref"] || "A1");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove commented out 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.

Done

padding:"1px 5px" ,

backgroundColor:"#E3EECB" }:{padding:"1px 5px" ,
backgroundColor:"#FFD0CA"}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Colors shouldn't be hardcoded. Please use the site theme

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

width: 150,
align:"center",
renderCell: (params) => (
<span style={params.value==="Yes" || params.value==="yes"?{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This ternary operator formatting is a bit awkward. Please use

condition
?
{ true case }
:
{ false case }

width: 150,
align:"center",
renderCell: (params) => (
<span style={params.value==="Yes" || params.value==="yes"?{
Copy link
Collaborator

@ckthiessen ckthiessen Mar 7, 2023

Choose a reason for hiding this comment

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

Prefer https://mui.com/material-ui/customization/typography/ for text. This also gives access to the sx property which you should use for styling

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done




//! Helper function for custom Pagination
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need this comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


//! Helper function for custom Pagination
function CustomPagination() {
const apiRef = useGridApiContext();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you building custom pagination? MUI Data Grid has built in pagination

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In order to make pagination style like what is required, I've built custom pagination.

// headerClassName: "super-app-theme--header",
// }));

//static Header
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}}>
{isFilePicked ? <div style={{ height: 570, width: "100%" }}><DataGrid getRowId={(row) => row.id} rows={searchResults.length > 0 ? searchResults : rows} columns={columns} pageSize={9} rowsPerPageOptions={[9]} components={{
Pagination: CustomPagination,
//! Toolbar: GridToolbar, //It's not required in our ptojec(I think It would be better if we add this feature)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this referring to?

amount: number,
) {
return { id, date, name, shipTo, paymentMethod, amount };
import React, { useState, useEffect, ChangeEvent,useContext } from "react";
Copy link
Collaborator

@ckthiessen ckthiessen Apr 13, 2023

Choose a reason for hiding this comment

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

There is some inconsistent spacing on this file. Please use VS Code Format Document on all the files you worked on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


const [isFilePicked, setIsFilePicked] = useState(false);

//* steps of add new order stepper
Copy link
Collaborator

@ckthiessen ckthiessen Apr 13, 2023

Choose a reason for hiding this comment

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

Remove this and the similar comments below please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done



const columns: GridColDef[] = [
{ field: "id", headerName: "No.", width: 50 , align:"center",headerAlign:"center"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

this object should be wrapped like all of the fields below it

import React from "react";
import Box from "@mui/material/Box";
import { Typography } from "@mui/material";

Copy link
Collaborator

Choose a reason for hiding this comment

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

A lot of unnecessary vertical whitespace (new lines) on this file and others. One new line between sections is enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

import Box from "@mui/material/Box";
import {FormControl,TextField } from "@mui/material";


Copy link
Collaborator

Choose a reason for hiding this comment

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

Too many newlines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

</FormControl>
</Box>
</Box>

Copy link
Collaborator

@ckthiessen ckthiessen Apr 13, 2023

Choose a reason for hiding this comment

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

Unnecessary newlines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

);
}
StepperProvider.propTypes = {
children: PropTypes.node.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use prop types. We are using typescript so use the typescript types instead. The type of children is React.ReactNode


"& .MuiStepLabel-label.Mui-completed.MuiStepLabel-alternativeLabel":
{
color: "black", // Just text label (COMPLETED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this and below inline comments

};

Steps.propTypes = {
setOpen: PropTypes.func.isRequired
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use typescript types instead of prop types

import Step from "@mui/material/Step";
import StepLabel from "@mui/material/StepLabel";
import StepConnector, { stepConnectorClasses } from "@mui/material/StepConnector";
import PropTypes from "prop-types";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete proptypes dependencies


];

const QontoConnector = styled(StepConnector)(() => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this name supposed to mean? What is Qonto? Please rename

<Box sx={{borderBottom: "solid", borderTop: "solid",borderWidth: 2, borderColor: "primary.main", p:2}} >
<FormControl >
<RadioGroup >
<FormControlLabel value="marketingEmail" control={<Radio />} label="Marketing Email" />
Copy link
Collaborator

@ckthiessen ckthiessen Apr 13, 2023

Choose a reason for hiding this comment

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

You should map a list of these values and labels instead of hardcoding all of the <FormControlLabel>s

import RadioGroup from "@mui/material/RadioGroup";
import FormControlLabel from "@mui/material/FormControlLabel";
import Radio from "@mui/material/Radio";

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary whitespace

<ButtonGroup sx ={{px:2}}>
<Button onClick={() => setAFAC(AFAC + 1)}>+</Button>
<Button >{AFAC}</Button>
<Button onClick={() => setAFAC(AFAC - 1)} >-</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if this gets set below 0? This should never be negative

@ckthiessen ckthiessen merged commit 9c79f37 into main May 6, 2023
@ckthiessen ckthiessen deleted the bhosseini/orders_page branch May 6, 2023 23:25
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.

4 participants