Frontend part A: user facing pages #78

AlexandreOndet · 2023-11-09T22:58:40Z

Merging after the infra changes is fine so it doesn't need to be in the same order as the rest of the backend

This PR contains:

all the frontend user facing routes
the text in english and french for all these pages
the http client to query the backend
the removal of webvitals
the updates of the dependencies

kr-matthews

Partial review - I haven't gone through Account or Rankings.
I also still don't have the repo running locally. But I assume it wouldn't work on this branch since it's just the front-end?

app/src/components/Provinces.tsx

kr-matthews · 2024-02-01T04:10:35Z

app/src/components/Provinces.tsx

+export const getProvincesWithNA: () => Province[] = () => {
+  return provinces.concat({
+    label: "N/A",
+    id: "na",
+    region: "N/A",
+    region_id: "na",
+  });
+};
+
+export const getProvinces = () => {
+  return provinces;
+};


Why are these functions, rather than directly exporting the 2 arrays as constants?

It was to avoid having all the lines duplicated just to have one with NA and one without. I added getProvinces to make the calls coherent.
But maybe my reasoning wasn't right I don't know. I'm always trying to avoid duplicated code when I can usually.

What do you think about a solution where we remove getProvinces but we keep getProvincesWithNA ?

I was thinking something like

export const provinces = [ // the provinces ] export const naProvince = { // ... } export const provincesWithNa = provinces.concat(naProvince)

but I might be overlooking something which prevents that from working the way I think it will.

app/src/components/Types.ts

kr-matthews · 2024-02-01T04:14:37Z

app/src/components/api.ts

+export const signIn = () => {
+  window.location.assign(new URL("/login", API_BASE_URL));
+};
+
+export const signOut = () => {
+  window.location.assign(new URL("/logout", API_BASE_URL));
+};


Is it possible to use react navigation to get to login/logout? I think that would be better practice than editing the window location directly.

That's what I tried first but for some reason it wasn't working. I agree with you that it would be more coherent to have it like the rest and I would be happy to change it if we find a way that works.

I don't recall at the moment how to change this. Maybe Jon can have a quick look when he has time.

app/src/httpClient.tsx

kr-matthews · 2024-02-01T04:17:58Z

app/src/pages/AdminPage.tsx

This is a util file, not a page nor a component.

This is going to be the Admin page in the part B PR (#79 ) so that's why the file is named that way

It might be worth keeping this isAdmin in a separate util file anyway, if it's going to be used in multiple components.
(I haven't looked at PR B yet.)

app/src/pages/AdminPage.tsx

kr-matthews

I reviewed Account. Still haven't looked closely at Rankings.

kr-matthews · 2024-02-07T04:36:41Z

app/src/pages/Account.tsx

+    case "HIDE_ALERT":
+      return {
+        ...state,
+        alert: false,
+      };


Might as well reset the alert to initialState, to avoid having the old type/content state hanging around and accidentally being revealed later.

kr-matthews · 2024-02-07T04:38:08Z

app/src/pages/Account.tsx

+
+const initialState: AlertState = {
+  alert: false,
+  alertType: "error",


Is there a more neutral option we could use as the initial/default state?

kr-matthews · 2024-02-07T04:39:41Z

app/src/pages/Account.tsx

+  ) || {
+    label: "N/A",
+    id: "na",
+    region: "N/A",
+    region_id: "na",
+  };


This is copy-pasted from the provinces.

kr-matthews · 2024-02-07T04:41:04Z

app/src/pages/Account.tsx

+    region: "N/A",
+    region_id: "na",
+  };
+  const defaultDOB = user?.dob ? dayjs(user.dob) : dayjs("2022-01-01");


Can we have a null default, or does this need to be a date? If it does need to be a date, then:

Suggested change

const defaultDOB = user?.dob ? dayjs(user.dob) : dayjs("2022-01-01");

const defaultDOB = dayjs(user?.dob ?? "2022-01-01");

kr-matthews · 2024-02-07T04:46:34Z

app/src/pages/Account.tsx

+      let tmpChipData = [];
+      for (let i = 0; i < user.roles.length; i++) {
+        tmpChipData.push({ key: i, label: user.roles[i] });
+      }


Generally in React, using something like map would be preferable to pushing to a variable. (And even as the code is, tmpChipData can be a const, it doesn't need to be a let.)

Suggested change

let tmpChipData = [];

for (let i = 0; i < user.roles.length; i++) {

tmpChipData.push({ key: i, label: user.roles[i] });

}

const chipData = user.roles.map((role) => ({ key: role, label: role }))

(I'm not 100% this is equivalent, but something like this.)

kr-matthews · 2024-02-07T04:53:15Z

app/src/pages/Account.tsx

+                  format="DD-MM-YYYY"
+                  InputProps={{
+                    style: {
+                      width: `${defaultEmail.length * 10 > 280 ? 145 : 280}px`,


Is the date field width meant to depend on the default email length?
It feels a bit weird to specify the width of input elements based on the default values. What exactly is the problem with fixed width?

kr-matthews · 2024-02-07T04:54:00Z

app/src/pages/Account.tsx

+              component="ul"
+            >
+              {chipData.map((data) => {
+                const color: chipColor | undefined =


This will always be a chipColor right? ie never undefined

kr-matthews · 2024-02-07T04:55:26Z

app/src/pages/AdminPage.tsx

It might be worth keeping this isAdmin in a separate util file anyway, if it's going to be used in multiple components.
(I haven't looked at PR B yet.)

kr-matthews · 2024-02-07T04:58:23Z

app/src/components/Provinces.tsx

+export const getProvincesWithNA: () => Province[] = () => {
+  return provinces.concat({
+    label: "N/A",
+    id: "na",
+    region: "N/A",
+    region_id: "na",
+  });
+};
+
+export const getProvinces = () => {
+  return provinces;
+};


I was thinking something like

export const provinces = [ // the provinces ] export const naProvince = { // ... } export const provincesWithNa = provinces.concat(naProvince)

but I might be overlooking something which prevents that from working the way I think it will.

kr-matthews · 2024-02-07T05:00:05Z

app/src/components/api.ts

+export const signIn = () => {
+  window.location.assign(new URL("/login", API_BASE_URL));
+};
+
+export const signOut = () => {
+  window.location.assign(new URL("/logout", API_BASE_URL));
+};


I don't recall at the moment how to change this. Maybe Jon can have a quick look when he has time.

kr-matthews

I've reviewed everything now, but I haven't run locally (but I assume it won't do anything without the backend PRs anyway?).

kr-matthews · 2024-02-11T23:28:31Z

app/src/pages/Rankings.tsx

+    if (province != null) {
+      console.log(ranking);
+      if (province.id === "qc") {
+        console.log("Vive le Québec libre!");
+      }
+    }


I'd be in favour of removing these.

kr-matthews · 2024-02-11T23:34:53Z

app/src/pages/Rankings.tsx

+        if (
+          eventId === null ||
+          province?.id === null ||
+          usingAverage === null ||


Isn't usingAverage always a boolean?
Can the conditional be simplified to if (!eventId || !province?.id) ...?

frontend user facing pages

ffa1343

AlexandreOndet requested a review from kr-matthews January 25, 2024 04:25

kr-matthews requested changes Feb 1, 2024

View reviewed changes

AlexandreOndet added 2 commits February 4, 2024 15:13

name refactoring after Kevin's review

3cd9f61

removed forgotten line

84a9416

kr-matthews reviewed Feb 7, 2024

View reviewed changes

kr-matthews requested changes Feb 11, 2024

View reviewed changes

	const defaultDOB = user?.dob ? dayjs(user.dob) : dayjs("2022-01-01");
	const defaultDOB = dayjs(user?.dob ?? "2022-01-01");

Frontend part A: user facing pages #78

Are you sure you want to change the base?

Frontend part A: user facing pages #78

Uh oh!

Conversation

AlexandreOndet commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kr-matthews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kr-matthews left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kr-matthews left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexandreOndet commented Nov 9, 2023 •

edited

Loading