Warning Header (Closes #205) #394
Merged
snedadah
merged 19 commits from warning-header into main 1 year ago
n3parikh
j285he
a258wang
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'warning-header'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Update:
In order to solve the issues discussed below, we decided to put the warnings in a json file, since they can be easily imported into a javascript file and webpack automatically bundles them with the client side app, so no static props is needed.
To get the warning data, since it is something that needs to be on all pages, it makes sense for the component to go in the app.js file (similar to the nav bar). However, next.js has a cumbersome issue that it does not support getStaticProps in the app.ts file (and getStaticProps only works in page files), thus we have no way of requesting the warning data easily in the <WarningHeader> Component. (https://github.com/vercel/next.js/discussions/10949)
Here is my solution:
The current problem with what I implemented is that it doesn’t build in the production environment:
Another advantage of allowing api’s is that maybe in the future we will implement other features where an api might be useful.
If you want to try it out, my branch works fine locally (when just running npm run dev or npm run build”)
Other possible solutions:
Use getInitialProps inside _app.js, but this has the downside that it will “disable Automatic Static Optimization in pages without Static Generation.” (https://nextjs.org/docs/advanced-features/custom-app), which I believe will slow the whole site down, but, we will be able to still use the old build command I think, I am not sure of the full effect of this.
Add the warning request to the getStaticProps of every single page, possibly through some wrapper component around every single page, this has the disadvantage that we need to do a lot of refactoring of all the pages of the site and cant use the intended “_app.ts” wrapper.
Add warning header only to the homepage (or maybe one or two other important pages).
So how should I handle this?
Thanks for the detailed research and explaining the tradeoffs, it's really helpful and well done!
Personally, I am hesitant to add an API. Having the website being fully static makes deploying really easy, and also means we don't need to worry about monintoring or making sure something hasn't crashed, has a bug etc.
In terms of alternatives, adding a warning header only on the homepage might be the best approach, but I'm not really sure yet. I think we should discuss it at sitdown this week and see what the rest of the team thinks.
--form-invalid: #9f616a;--warning-background: #dd0014;--warning-text: #ffffff;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.
import styles from "./WarningHeader.module.css";// Had to add these functions here, since we cannot use import the file from ./lib/events since that uses libraries only on the serversideWe should probably move these functions to utils in that case, and then make both events and this file import from there
if (!startDate ||!endDate ||isNaN(startDate.getTime()) || // this checks if the parsed date is not valid (eg. wrong format)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?
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.
export function WarningHeader() {const warning = getCurrentWarning();return !warning || warning.message == "" ? (I think it's safe to assume that if a warning is not null, message will not be
""Doing input sanitation is good practice in general, but it doesn't give us a lot of benefit in most of our usecases. Input sanitation is more or less required when we are dealing with arbitrary input from random users. I think in our case, we are our own users, and we can sort of figure out what the 'input' should be. That plus code reviews make it hard to accidentally add something that's wrong.
on another note, you should use
foo == nullto do null checks.!foowill work on falsy values like 0, "" as well, where as foo == null only returns true forundefinedandnull.If you are doing any comparison other than null (^) you should always use ===. This avoids some pitfalls of the JS language.
const warning = getCurrentWarning();return !warning || warning.message == "" ? (<></>you can return null here
const today = new Date();let currentWarning: Warning | null = null;warnings.forEach((warning) => {instead of setting the
currentWarning, you should use a declarative approach:background-color: var(--warning-background);padding: calc(5rem / 16);color: var(--warning-text);font-size: calc(15rem / 16);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 :) )
Looks good overall @snedadah! 🎉
Can you also update the PR comment with why we are using JSON? Don't delete what's already there, just add something on top of it so our future selves have some context about why we made this decision.
PS: Your detailed description is really nice!
isNaN(startDate.getTime()) || // this checks if the parsed date is not valid (eg. wrong format), since getLocalDateFromEST fails with invalid datesisNaN(endDate.getTime())) {console.error("WARNING WITH INVALID DATES:", warning);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.
WIP: Warning Header (Closes #205)to Warning Header (Closes #205) 1 year agoisNaN(startDate.getTime()) || // this checks if the parsed date is not valid (eg. wrong format), since getLocalDateFromEST fails with invalid datesisNaN(endDate.getTime())) {// this warning is not valid, dont try to call getLocalDateFromESTDo 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.
// check if current warning is in rangereturn (startDate.getTime() < today.getTime() &&endDate.getTime() > today.getTime()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 :)});if (currentWarnings.length > 1) {// we should not throw here, since this could happen a date far after the push was made, and no one would know they site is down unless someone visitsThis 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.
);}return currentWarnings.length == 0 ? null : currentWarnings[0];Is there a reason why we're using
==instead of===?export function WarningHeader() {const warning = getCurrentWarning();return warning == null ? 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 havenullshow up twice makes my brain go 🤔, even though this works just fine. Maybe we could consider usingor
if either of these options seem more readable/less awkward?
The reason behind doing it this way was:
#394
and #394
Also, if you use
===, thenundefined === nullis false, butundefined == nullis 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,
warningis always going to be either aWarningobject ornull, so in this situation checking ifwarningis null is equivalent to checking if it is falsy. Both of the options I suggested will returnnullifwarningisnull. 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).Looks good to me! Thank you Shahan for your thoughtfulness and your responsiveness to feedback.
bb78a3d53dinto main 1 year agoReviewers
bb78a3d53d.