Warning Header (Closes #205) #394

Merged
snedadah merged 19 commits from warning-header into main 2022-02-22 23:29:55 -05:00
10 changed files with 112 additions and 15 deletions

View File

@ -34,6 +34,8 @@ export const PALETTE_NAMES = [
"--text",
"--form-invalid",
"--warning-background",
"--warning-text",
"--input-background",
"--input-placeholder-text",

View File

@ -0,0 +1,12 @@
.warning{
background-color: var(--warning-background);
padding: calc(6rem / 16);
color: var(--warning-text);
font-size: calc(16rem / 16);
snedadah marked this conversation as resolved Outdated

this is an opinion with no justification whatsoever, so feel free to ignore this if you don't feel the same way, I'm not gonna be offended :)


I like to use even number sizes. So either 14px or 16px would be good.

this is an opinion with no justification whatsoever, so feel free to ignore this if you don't feel the same way, I'm not gonna be offended :) --- I like to use even number sizes. So either 14px or 16px would be good.

I actually prefer it at 6 and 16 too! (not since I like even numbers, but just in terms of the layout :) )

I actually prefer it at 6 and 16 too! (not since I like even numbers, but just in terms of the layout :) )
text-align: center;
opacity: 1;
/* The following are for a smooth fade in if there ever is a loading required for the warning, is not needed currently */
/* max-height: 500px;
/* transition: max-height 1000ms ease-in, padding 100ms ease-in; */
}

View File

@ -0,0 +1,60 @@
import { parse } from "date-fns";
import React from "react";
import warnings from "../content/warnings/warnings.json";
import { DATE_FORMAT, getLocalDateFromEST } from "../utils";
import styles from "./WarningHeader.module.css";
interface Warning {
snedadah marked this conversation as resolved
Review

We should probably move these functions to utils in that case, and then make both events and this file import from there

We should probably move these functions to utils in that case, and then make both events and this file import from there
message: string;
startDate: string;
endDate: string;
}
function getCurrentWarning(): Warning | null {
const today = new Date();
const currentWarnings: Warning[] = warnings.filter((warning) => {
// convert dates to date objects in EST time zone
let startDate = parse(warning.startDate, DATE_FORMAT, new Date());
let endDate = parse(warning.endDate, DATE_FORMAT, new Date());
if (
!startDate ||
!endDate ||
isNaN(startDate.getTime()) || // this checks if the parsed date is not valid (eg. wrong format), since getLocalDateFromEST fails with invalid dates
isNaN(endDate.getTime())
) {
throw new Error('WARNING WITH INVALID DATES: "' + warning.message + '"');
snedadah marked this conversation as resolved
Review

Do we really need this comment here? I personally think the comment on line 25, along with the general structure of the code, are already sufficient to convey what is happening and why.

Do we really need this comment here? I personally think the comment on line 25, along with the general structure of the code, are already sufficient to convey what is happening and why.
}
snedadah marked this conversation as resolved Outdated

instead of setting the currentWarning, you should use a declarative approach:

const currentWarnings = warnings.filter(warning => ...do the thing to filter by date...);
instead of setting the `currentWarning`, you should use a declarative approach: ```ts const currentWarnings = warnings.filter(warning => ...do the thing to filter by date...); ```
startDate = getLocalDateFromEST(startDate);
snedadah marked this conversation as resolved Outdated

I would recommend throwing instead of logging the error. Just logging the error makes us deal with things that aren't defined. If we throw, we don't have to deal with undefined behavior.

I would recommend throwing instead of logging the error. Just logging the error makes us deal with things that aren't defined. If we throw, we don't have to deal with undefined behavior.
endDate = getLocalDateFromEST(endDate);
return (
startDate.getTime() <= today.getTime() &&
endDate.getTime() >= today.getTime()
);
});
snedadah marked this conversation as resolved Outdated

Nitpick: would it make more sense to use <= and >= instead of < and >?

As well, is the comment on line 35 really necessary? I personally think the code is already descriptive enough on its own.

Nitpick: would it make more sense to use `<=` and `>=` instead of `<` and `>`? As well, is the comment on line 35 really necessary? I personally think the code is already descriptive enough on its own.

You are correct, it does make more sense to use >=, I'll update this, however getTime() returns a time in milliseconds, so the difference between > and >= will only be evident for one millisecond :)

You are correct, it does make more sense to use >=, I'll update this, however getTime() returns a time in milliseconds, so the difference between `>` and `>=` will only be evident for one millisecond :)
if (currentWarnings.length > 1) {
// If more than one warning is scheduled, log an error to the console. We cannot throw an error, since the site would go down on the live

No need to do all of these checks imo, the parsing library should take care of that.

No need to do all of these checks imo, the parsing library should take care of that.

The reason I added this check, was that the function getLocalDateFromEST would throw a fatal error if it was called with an invalid date (the site would not even load, at least in development mode)

I'm not sure though, since maybe this is good behaviour to have if the user accidentally made a mistake in one of the dates, they will be sure to know if they try to run the site.

Please let me know what behaviour is prefered?

The reason I added this check, was that the function getLocalDateFromEST would throw a fatal error if it was called with an invalid date (the site would not even load, at least in development mode) I'm not sure though, since maybe this is good behaviour to have if the user accidentally made a mistake in one of the dates, they will be sure to know if they try to run the site. Please let me know what behaviour is prefered?

yes, we should fail on an invalid state. In general it's better to fail while building than while running.

that being said we if the fatal error wasn't making it obvious that it's happening because of an invalid date, we should add some more information there.

yes, we should fail on an invalid state. In general it's better to fail while building than while running. that being said we if the fatal error wasn't making it obvious that it's happening because of an invalid date, we should add some more information there.
// website, on the day when more than one warning is scheduled.
console.error(
snedadah marked this conversation as resolved Outdated

This comment confused me the first time I read it. Maybe we could consider wording it more succinctly, eg. // log an error instead of throwing, so the site can still be built.

This comment confused me the first time I read it. Maybe we could consider wording it more succinctly, eg. `// log an error instead of throwing, so the site can still be built`.

This is actually won't effect the site building, since this code is all client side.

This code will get run everytime someone visits the site. If we threw an error, the following could happen:

After a push to the repositry, the site will build fine and look completly fine when deployed. However, if on someday in the future, there is two warnings scheduled on that same day, on that day (and only on that day, not earlier), the site will stop working. (when someone visits the site, it would not load), and we would have no idea unless we actually visit the site.

I'll make sure to make the comment a bit more clear.

This is actually won't effect the site building, since this code is all client side. This code will get run everytime someone visits the site. If we threw an error, the following could happen: After a push to the repositry, the site will build fine and look completly fine when deployed. However, if on someday in the future, there is two warnings scheduled on that same day, on that day (and only on that day, not earlier), the site will stop working. (when someone visits the site, it would not load), and we would have no idea unless we actually visit the site. I'll make sure to make the comment a bit more clear.
"ERROR: MORE THAN ONE WARNING SCHEDULED CURRENTLY! ",
currentWarnings
);
}
return currentWarnings.length === 0 ? null : currentWarnings[0];
}
snedadah marked this conversation as resolved Outdated

Is there a reason why we're using == instead of ===?

Is there a reason why we're using `==` instead of `===`?
export function WarningHeader() {
const warning = getCurrentWarning();
if (warning == null) {
return null;

Similar to above - is there a reason why we're using == instead of ===?

Also, a nitpick: personally it looks a little weird to me that we have ... == null ? null : ... - the fact that we have null show up twice makes my brain go 🤔, even though this works just fine. Maybe we could consider using

return warning ? <div>...</div> : null;

or

return (warning && <div>...</div>);

if either of these options seem more readable/less awkward?

Similar to above - is there a reason why we're using `==` instead of `===`? Also, a nitpick: personally it looks a little weird to me that we have `... == null ? null : ...` - the fact that we have `null` show up twice makes my brain go 🤔, even though this works just fine. Maybe we could consider using ```react return warning ? <div>...</div> : null; ``` or ```react return (warning && <div>...</div>); ``` if either of these options seem more readable/less awkward?

The reason behind doing it this way was:
#394 (comment)
and #394 (comment)

Also, if you use ===, then undefined === null is false, but undefined == null is true, so the second way is prefered when doing null checks.

The reason behind doing it this way was: https://git.csclub.uwaterloo.ca/www/www-new/pulls/394#issuecomment-7393 and https://git.csclub.uwaterloo.ca/www/www-new/pulls/394#issuecomment-7390 Also, if you use `===`, then `undefined === null` is false, but `undefined == null` is true, so the second way is prefered when doing null checks.

Ahh okay, good point about the null checks!

As for the other point, the way I see it, warning is always going to be either a Warning object or null, so in this situation checking if warning is null is equivalent to checking if it is falsy. Both of the options I suggested will return null if warning is null. That being said, this is honestly just a matter of personal preference, so I'll let you decide what you prefer (unless someone else wants to weigh in with their opinion).

Ahh okay, good point about the null checks! As for the other point, the way I see it, `warning` is always going to be either a `Warning` object or `null`, so in this situation checking if `warning` is null is equivalent to checking if it is falsy. Both of the options I suggested will return `null` if `warning` is `null`. That being said, this is honestly just a matter of personal preference, so I'll let you decide what you prefer (unless someone else wants to weigh in with their opinion).
}
return <div className={styles.warning}>{warning.message}</div>;
}

View File

@ -0,0 +1,12 @@
[
{
"startDate": "February 15 2022 00:00",
"endDate": "February 20 2022 18:00",
"message": "Warning: There will be a scheduled system maintenance on February 17 from 9pm to 12pm EST"
},
{
"startDate": "January 29 2022 21:00",
"endDate": "January 30 2022 18:00",
"message": "This is a sample warning"
}
]

View File

@ -2,14 +2,19 @@ import fs from "fs/promises";
import path from "path";
import { parse } from "date-fns";
import { utcToZonedTime, zonedTimeToUtc } from "date-fns-tz";
import matter from "gray-matter";
import { MDXRemoteSerializeResult } from "next-mdx-remote";
import { serialize } from "next-mdx-remote/serialize";
import type { Props } from "../pages/events/[year]/[term]/index";
// do not use alias "@/utils" as generate-calendar imports a function from this file and ts-node is not compatible
import { Term, TERMS, isTerm } from "../utils";
import {
Term,
TERMS,
isTerm,
DATE_FORMAT,
getLocalDateFromEST,
} from "../utils";
const EVENTS_PATH = path.join("content", "events");
@ -55,8 +60,6 @@ export interface Event {
metadata: Metadata;
}
export const DATE_FORMAT = "MMMM dd yyyy HH:mm";
export async function getEventBySlug(
year: string,
term: Term,
@ -284,12 +287,3 @@ function getFutureTerm(year: string, term: Term): { year: string; term: Term } {
term: TERMS[index + 1],
};
}
// The date that's returned should be in local time
export function getLocalDateFromEST(date: Date) {
return utcToZonedTime(
// The parsed date is in EST
zonedTimeToUtc(date, "America/Toronto"),
Intl.DateTimeFormat().resolvedOptions().timeZone
);
}

View File

@ -8,7 +8,7 @@ import { serialize } from "next-mdx-remote/serialize";
import { isTerm, Term, TERMS } from "@/utils";
import { DATE_FORMAT, getLocalDateFromEST } from "./events";
import { DATE_FORMAT, getLocalDateFromEST } from "../utils";
export const NEWS_PATH = path.join("content", "news");

View File

@ -22,6 +22,8 @@ body {
--text: #000000;
--form-invalid: #9f616a;
--warning-background: #dd0014;
--warning-text: #ffffff;
snedadah marked this conversation as resolved
Review

Make sure to add these colors in our theme palette as well. That way it will show up in the themer and will be totally customizable!

Themer: https://csclub.uwaterloo.ca/themer

Adding a color to the palette should be as easy as adding a string to this array. If that doesn't work, let me know and we can figure something out.

Make sure to add these colors in our theme palette as well. That way it will show up in the themer and will be totally customizable! Themer: https://csclub.uwaterloo.ca/themer Adding a color to the palette should be as easy as adding a string to [this array](https://git.csclub.uwaterloo.ca/www/www-new/src/branch/main/components/Theme.tsx#L18). If that doesn't work, let me know and we can figure something out.
--input-background: #f0f0f0;
--input-placeholder-text: #bbbbbb;

View File

@ -20,6 +20,7 @@ import {
} from "@/components/ShapesBackground";
import { Table } from "@/components/Table";
import { ThemeProvider } from "@/components/Theme";
import { WarningHeader } from "@/components/WarningHeader";
import styles from "./_app.module.css";
@ -44,6 +45,7 @@ export default function App({ Component, pageProps }: AppProps): JSX.Element {
}}
>
<div className={styles.appContainer}>
<WarningHeader />
<Navbar />
{/* Wrapping content with a div to allow for a display: block parent */}
<div className={styles.contentContainer}>

View File

@ -3,8 +3,9 @@ import path from "path";
import { format } from "date-fns";
import { DATE_FORMAT } from "@/utils";
import {
DATE_FORMAT,
getEventsByTerm,
getEventTermsByYear,
getEventYears,

View File

@ -1,5 +1,8 @@
import { utcToZonedTime, zonedTimeToUtc } from "date-fns-tz";
export const TERMS = ["winter", "spring", "fall"] as const;
export type Term = typeof TERMS[number];
export const DATE_FORMAT = "MMMM dd yyyy HH:mm";
// https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
export function isTerm(x: string): x is Term {
@ -9,3 +12,12 @@ export function isTerm(x: string): x is Term {
export function capitalize(str: string) {
return str.slice(0, 1).toUpperCase() + str.slice(1);
}
// Converts a date to local time
export function getLocalDateFromEST(date: Date): Date {
return utcToZonedTime(
// The date parameter is in EST
zonedTimeToUtc(date, "America/Toronto"),
Intl.DateTimeFormat().resolvedOptions().timeZone
);
}