CRLSCSClub / CRLSTime

A clock for showing the CRLS schedule during the school day
6 stars 10 forks source link

Refactored code (merge into testing) #30

Closed psvenk closed 5 years ago

psvenk commented 5 years ago

I saw that in commit 4448327e4866c50d4bc761631872c2e92a3e3102, the JavaScript was moved from the index.html file into two separate files based on the function of the code, schedule.js and draw.js (along with some other improvements, see pull request #25). This commit was later reverted, which means that all of the JavaScript code is now in the main HTML file. This makes it difficult for an onlooker to read through the main HTML file (because it contains not just HTML, but also JavaScript code), and it means that code that controls the schedule and code that controls drawing elements on the page are lumped in one <script> tag in such a way that it is difficult to find all of the code that does another thing.

Therefore, I have edited the files schedule.js and draw.js to incorporate the changes made since that commit was reverted. This way, we can have the latest improvements to the CRLSTime code while keeping the code readable and maintainable (by keeping it in files based on their function, e.g. index.html is the HTML markup, schedule.js contains the schedule logic, and draw.js contains the drawing logic). For more information, see pull request #25.

I have not yet fully tested the code, so please do not vote for the code unless you have tested it (with the exception of @dougmcg, because I have requested for you to check that it works on your iPhone 4s). Because I have only worked on the code at home after school, the application shows a regular clock instead of a block in the school day.* I will remove the "draft" status once I have tested this during the school day. You can help test the code by going to https://psvenk.github.io/CRLSTime/ during the school day. Please vote for (review) the pull request if you find it to work (considering that I have taken it out of draft mode and that you agree with the pull request; if you disagree, I am truly interested in knowing why you disagree).

* I have tried overriding the variable representing the current time in order to be able to see what the clock would look like at some time in the school day, but this was not easy from the JavaScript console (Ctrl+Shift+J in Chrome/Chromium). I then tried changing the code a bit to allow overriding this variable, but this was not easily done due to the way that the code is written. I plan on rewriting parts of the code to accommodate this type of override (via a JavaScript function, not anything user-facing like a button) some time in the future.

psvenk commented 5 years ago

If you want to test the new code, you can now go to https://psvenk.github.io/CRLSTime/. Please note that this URL doesn't necessarily always contain the latest changes—I just created it so that I can link to it in pull requests. The notice at the top ("This version isn't stable") and the title of the webpage being "CRLSTime-testing" are not included in this pull request, but are on that webpage because I thought that someone might go to my webpage instead of the official page, crlscs.club.

psvenk commented 5 years ago

I just noticed that replacing var statements with let statements causes the clock to break on older browsers (#26). I'll change the let statements back to var statements.

psvenk commented 5 years ago

The code now works in Firefox 40, an older browser that doesn't support let. I don't know if we should remove the const statements and replace them with var statements, because old versions of Internet Explorer are the only browsers that don't support const at all. I don't think that we should target old versions of Internet Explorer because any site hosted by GitHub won't even load in IE9 and below (see bottom screenshot). I have not tested IE10, but the only people using IE10 are on Windows 7 or 8, which means that they can upgrade to IE11, but people using Windows Vista or earlier cannot upgrade to IE10 or IE11.

Before: Screenshot at 2019-03-08 22-05-48

After: Screenshot at 2019-03-08 22-17-12

IE9 not loading any GitHub webpages: image

psvenk commented 5 years ago

@dougmcg I would like to make sure that this works on your iPhone 4s.

psvenk commented 5 years ago

I tested this code today, and it worked flawlessly, so I am now merging because my understanding is that the testing branch need not be rigorously tested.

Screenshots: IMG_1992 IMG_1993 IMG_1994 IMG_1995 IMG_1996 IMG_1997 IMG_1998