[M1_TR-212] Markup for M1_TR-211 #11

nicoleamber · 2023-09-04T10:31:05Z

Related Issue

Backlog - Markup for M1_TR-211

Context

Created markup for Job List page

Commands:

Run the ff commands

  cd web
  yarn install

Go to http://localhost:3000/

Test Executed

Unit Test
Functional Test

Screenshot

Screencast.2023-09-07.11-07-01.webm

Nature of task

New Implementation
Bug
Refactoring

Risks

Affecting other functions

Notes

Filter buttons take a few seconds to initially load its custom styles.

jeremiahC

Please use 2 space indentation not 4 space

jeremiahC · 2023-09-19T10:05:18Z

web/src/components/atoms/Pagination/index.tsx

@nicoleamber Pagination should be a molecule since it compose of multiple buttons

jeremiahC · 2023-09-19T10:12:55Z

web/src/app/job/hooks.ts

+export const useJobListContext = (): JobTable => {
+    const jobs = useContext(JobListContext);
+
+    if (jobs === undefined) {
+        throw new Error('Missing JobListContext');
+    }
+
+    return jobs;
+};


@nicoleamber I think we should not export, let's manage context within hooks and pass what we need in the return statement. Also, I can see the context is not used in this hooks so only use it where it is needed.

jeremiahC

@nicoleamber Please fix indentation as well

jeremiahC · 2023-09-21T09:45:50Z

web/src/components/molecules/NavBarItem.tsx

Please put it in a folder same as others

jeremiahC

From next time if markup task, I want to request to not include the hooks for now or context, it's okay na display lang sa, kay daghan kaayo i review

jeremiahC · 2023-09-22T09:06:52Z

web/src/app/wrapper.tsx

just request, why no use template.js instead of wrapper?
https://nextjs.org/docs/app/building-your-application/routing/pages-and-layouts#templates

jeremiahC

better to organize the routes like this

/job/list
/job/detail/{id}

since route order is depending on the component tree. So the file structure will look like this

+ job
   + list
      - context.js
      - hooks.js
      - page.js

jeremiahC · 2023-09-22T09:10:53Z

web/src/styles/Filter.module.css

please move styles to assets

jeremiahC · 2023-09-25T09:38:55Z

web/src/components/atoms/StatusChip/index.tsx

+    <Chip
+      label={label}
+      sx={{
+        backgroundColor: ChipColors[label as string] ?? '#65707b33',


Please define every hex value colors, so that it would be easily identified by developers.

jeremiahC · 2023-09-25T09:39:34Z

web/src/components/molecules/CreatedDateRangeFilter/hooks.ts

+    setStartDate(date);
+  };
+
+  const handleEndDateChange = (date: Moment | null): void => {


do we need to use union types? This function requires date always right?

@jeremiahC Yes sir, there would be a type mismatch without the null as seen in this error message.

It's also possible to clear the date fields (i.e. when we no longer want to filter jobs by date) using null as the on change value. I'll include that change here since I implemented it in the FE task.

jeremiahC · 2023-09-25T09:39:39Z

web/src/components/molecules/CreatedDateRangeFilter/hooks.ts

+  const [startDate, setStartDate] = useState<Moment | null>();
+  const [endDate, setEndDate] = useState<Moment | null>();
+
+  const handleStartDateChange = (date: Moment | null): void => {


do we need to use union types? This function requires date always right?

jeremiahC · 2023-09-25T09:40:11Z

web/src/components/molecules/CreatedDateRangeFilter/index.tsx

+  const { startDate, handleStartDateChange, endDate, handleEndDateChange } =
+    useHooks();


format each variable by line

jeremiahC · 2023-09-25T09:45:34Z

web/src/components/molecules/NavBarItem/index.tsx

+  linkTo = '/'
+}) => {
+  const pathname = usePathname();
+  active = pathname === linkTo ? true : false;


better if active is immutable, pathname === linkTo should be handled above this component. This component will just receive true or false.

jeremiahC · 2023-09-25T09:47:09Z

web/src/components/organisms/HeaderProfile/index.tsx

+    setOpen((prevOpen) => !prevOpen);
+  };
+
+  const handleClose = (event: Event | React.SyntheticEvent) => {


why do we need to use union types here?

…emoved unnecessary unions

nicoleamber force-pushed the feature/M1_TR-212 branch 2 times, most recently from 267fb6a to ac3107a Compare September 6, 2023 10:29

[M1_TR-212] Markup for M1_TR-211

f5feb02

nicoleamber force-pushed the feature/M1_TR-212 branch from ac3107a to f5feb02 Compare September 7, 2023 03:04

nicoleamber self-assigned this Sep 7, 2023

nicoleamber requested a review from jeremiahC September 7, 2023 03:15

nicoleamber marked this pull request as ready for review September 7, 2023 03:15

[M1_TR-212] Refactored structure

7ca69fd

jeremiahC requested changes Sep 19, 2023

View reviewed changes

jeremiahC reviewed Sep 19, 2023

View reviewed changes

jeremiahC requested changes Sep 19, 2023

View reviewed changes

[M1_TR-212] Fixed indentation and moved context to hooks

6b414f8

nicoleamber requested a review from jeremiahC September 20, 2023 01:31

nicoleamber added 2 commits September 20, 2023 14:45

[M1_TR-212] Convert indent to space

68e7998

[M1_TR-212] Add indent rule

86e5426

nicoleamber force-pushed the feature/M1_TR-212 branch from 060a487 to 86e5426 Compare September 20, 2023 07:17

jeremiahC reviewed Sep 21, 2023

View reviewed changes

[M1_TR-212] Moved components into folders

c38cc39

jeremiahC reviewed Sep 22, 2023

View reviewed changes

jeremiahC requested changes Sep 22, 2023

View reviewed changes

jeremiahC reviewed Sep 22, 2023

View reviewed changes

web/src/styles/Filter.module.css Outdated

Copy link

Contributor

jeremiahC Sep 22, 2023

Choose a reason for hiding this comment

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

please move styles to assets

nicoleamber reacted with rocket emoji

[M1_TR-212] Reorganized file structure

1730d00

jeremiahC reviewed Sep 25, 2023

View reviewed changes

[M1_TR-212] Added custom colors, updated navbar items active state, r…

470017e

…emoved unnecessary unions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[M1_TR-212] Markup for M1_TR-211 #11

[M1_TR-212] Markup for M1_TR-211 #11

nicoleamber commented Sep 4, 2023 •

edited

Loading

jeremiahC left a comment

jeremiahC Sep 19, 2023 •

edited

Loading

jeremiahC Sep 19, 2023 •

edited

Loading

jeremiahC left a comment

jeremiahC Sep 21, 2023

jeremiahC left a comment

jeremiahC Sep 22, 2023

jeremiahC left a comment

jeremiahC Sep 22, 2023

jeremiahC Sep 25, 2023

jeremiahC Sep 25, 2023

nicoleamber Sep 25, 2023

jeremiahC Sep 25, 2023

jeremiahC Sep 25, 2023

jeremiahC Sep 25, 2023

jeremiahC Sep 25, 2023

		const { startDate, handleStartDateChange, endDate, handleEndDateChange } =
		useHooks();

[M1_TR-212] Markup for M1_TR-211 #11

Are you sure you want to change the base?

[M1_TR-212] Markup for M1_TR-211 #11

Conversation

nicoleamber commented Sep 4, 2023 • edited Loading

Related Issue

Context

Commands:

Test Executed

Screenshot

Nature of task

Risks

Notes

jeremiahC left a comment

Choose a reason for hiding this comment

jeremiahC Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

jeremiahC Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

jeremiahC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiahC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiahC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoleamber commented Sep 4, 2023 •

edited

Loading

jeremiahC Sep 19, 2023 •

edited

Loading

jeremiahC Sep 19, 2023 •

edited

Loading