[DO NOT MERGE] - PR Just as a review base, to collect practical reviews #15
Conversation
AIDA Explorer , AIDA Header,AIDA Sign In Google/LinkedIn/Facebook Open Aid addfilter Button , Open AID Year Display Graph Button, Aida Log Out Button
Open Aid Share Button , Open Aid Download Button , Open Aid Select Language Button , Open Aid MenuButton, Data explorer Search button
Data file, Stories,Component itself and Index file
Table Component file, Table Stories file , index file and data file
DataGrid Component, Datagrid Stories , index file, data file for the datagrid
Sizes, Error, Success
Button components
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
There was a problem hiding this comment.
ZDS general review notes
- remove unused imports
- organise assets in sub-directories as per project
- move interfaces from actual component file to data.ts file
- story file names must be relevant to the component it showcases
- theme files look similar; also avoid using
.jsor.jsxfiles; instead use.tsor.tsx - the inline SVG better to be put in actual SVG files and import them to use in other components
- generic components instead of using "My" convention, let's use something like "ZDS"
- try to use absolute imports instead of relative imports - https://medium.com/geekculture/making-life-easier-with-absolute-imports-react-in-javascript-and-typescript-bbdab8a8a3a1
- also wherever possible import single component default imports from packages
- e.g.
import { Box } from "@mui/material";toimport Box from "@mui/material/Box";
- e.g.
There was a problem hiding this comment.
Let's not use package-lock.json & yarn.lock files. Let's all use yarn to stick to yarn.lock
@sylvanr @okorie2 @Psami-wondah @FAUSTMANNSTEF
| aria-label={props.ariaLabel} | ||
| style={{ textAlign: textAlign }} | ||
| data-cy={props.datacy} | ||
| children={props.children} |
There was a problem hiding this comment.
Pass props.children into the component's tag element:
<IconButton>{props.children}</IconButton>
| import colors from "../../theme/colors"; | ||
| import React, { useState } from "react"; | ||
| import LogoutIcon from "@mui/icons-material/Logout"; | ||
| import { |
There was a problem hiding this comment.
Import them separately as the LogoutIcon above
| const BORDER_STYLES = { | ||
| RED: "2px solid red", | ||
| BLUE: "2px solid blue", | ||
| } as const; |
| ArrowForward, | ||
| Share, | ||
| LinkedIn, | ||
| Microsoft, |
| import type { Meta, StoryObj } from "@storybook/react"; | ||
| import { fn } from "@storybook/test"; | ||
| import Button from "./myButton"; | ||
| import { ReactComponent as GoogleIcon } from "../../assets/vectors/jsx/HeaderSignInViewGoogle.svg"; |
There was a problem hiding this comment.
we should use absolute imports for components being imported from a different directory. Or for every import if possible
| IconComponent={props.IconComponent} | ||
| labelId={`${id}-label`} | ||
| onChange={props.onChange} | ||
| renderValue={(selected: string | string[]) => { |
There was a problem hiding this comment.
declare this as a function at the top instead, and call the function here. Let's avoid inline function declarations
| layout: "centered", | ||
| }, | ||
| tags: ["autodocs"], | ||
| argTypes: { |
There was a problem hiding this comment.
These arg types can be inferred from the component props type yh?
| display?: string; | ||
| borderRadius?: string; | ||
| textTransform?: string; | ||
| opacity?: string; |
There was a problem hiding this comment.
Most of these styles are already defined in SxProps I think.
No description provided.