Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Massive Changes and Overhauls #76

Merged
merged 36 commits into from
Sep 16, 2023
Merged

Massive Changes and Overhauls #76

merged 36 commits into from
Sep 16, 2023

Conversation

aschmitigal22
Copy link
Collaborator

The Changes

There are a lot of changes in this pull request. Some are big changes like external pages and others are small like new settings or formatting changes.

Let's get into the big ones first:

New File Structure

The file structure of the project has been overhauled to be more organized and less "a bunch of files in one folder". The main files that need to sit at the root are still there but all other reference files have been moved into sub-folders.

/
├ index.html
├ offline.html
├ readme.md
+ robots.txt 
├ firebase.json
└ .gitignore

The robots.txt was added for completeness but not strictly necessary, although I might add a sitemap.xml in the future.
The rest of the files have been divided into src for scripts, public for static files, pages for the pages, templates for the Vue templates, and data for the JSON data.

data
├ events.json
├ languages.json
└ schedule.json
pages
├ calendar.html
├ calendarDay.html
├ classNames.html
├ data.html
├ now.html
└ settings.html
templates
├ periodInformation.html
└ periodList.html
public
├ favicon.ico
├ faviconAppIcon.png
├ faviconClear.png
├ faviconLarge.png
├ faviconLargeLarge.png
├ manifest.json
└ style.css
src
-├ vueScript.js
+├ index.js
-├ scheduleFormatting.js
+├ schedule.js
├ serviceWorker.js
└settings.js

These files were renamed for clarity and all file references and links have been updated to match.

External Pages & Templates

As you may have noticed there is now a pages directory with html pages inside. These pages and templates were extracted from the main index.html file for a more simplistic developer experience and a slimming of the index.html file down a bit. In their place, there is a div that holds the pages that get added as children.

<div id="pages">
  <!-- Loads pages from external files -->
</div>

Then the code to load the pages is in the index.js file

// Define all the pages that will be used
export const Pages = {
  Now: "now",
  Calendar: "calendar",
  CalendarDay: "calendarDay",
  Settings: "settings",
  ClassNames: "classNames",
  Data: "data",
  values: () => Object.values(Pages).filter((v) => typeof v === "string")
};
// Then add the pages to the page.
const pagesElement = document.getElementById("pages");
await Promise.allSettled(
  Pages.map((page) =>
    fetch(`/pages/${page}.html`)
      .then((response) => response.text())
      .then((html) => ({ page, html }))
      .then((page) => pagesElement.insertAdjacentHTML("beforeend", `<div id="${page.page}-page" v-if="currentPage == '${page.page}'">${page.html}</div>`))
));

This simplifies the process of adding pages because now all you need to do is add it to the Pages object and create a file by the same name in the pages folder.
This all works the same for the templates except they get appended to the bottom of the page because they get referenced by id in the code.

export const Templates = {
  PeriodInformation: "periodInformation",
  PeriodList: "periodList",
  values: () => Object.values(Templates).filter((v) => typeof v === "string")
};

Settings Tweaking

The settings.js file got reworked a little. Firstly you will see JSDoc comments in this file for type definitions for the settings object and the types of settings. This was mostly an experiment to see how well they work because I miss Typescript. We can remove them if needed and I will cover them more in the future improvements section.

The settings now reference themselves by key in the settings object. Plus the defaults have also been moved into the settings object.

export const settings = {
- grade: {
+ GRADE_LEVEL: {
- setting: "grade",
    mode: "dropdown",
    options: ["GRADE_7", "GRADE_8", "GRADE_9", "GRADE_10", "GRADE_11", "GRADE_12"],
+   default: "GRADE_9",
    new: false,
    experimental: false,
    reload: true,
  },
// ...
}

Toggle settings have been changed from 2D arrays with booleans and strings to an array of objects with a value and text. The value can be anything because I have removed all truthy references to it in the HTML. As an example, the EMOJI_DISTROBUTION setting uses strings for values.

EXTRA_PERIODS: {
  mode: "toggle",
  options: [
    { value: true, text: "ON" },
    { value: false, text: "OFF" },
  ],
// ...
}

The sliders have been generalized by removing the color-slider mode and replacing it with just slider. In the HTML for settings, there is a specific exception for the COLOR_THEME settings to use extra CSS for the slider.
Lastly for settings, the userSettings has been consolidated at the bottom to pull the defaults from the settings.

Schedule Functions

The schdeule.js file has been simplified when checking for the data loading versions and the language loading offers warning messages when languages are missing keys that are present in ENGLISH.
The getSchedule function has been broken into subparts that can be called independently reducing the calls to getSchedule for simple checks like the schedule or if on break. The function also now returns an object with more information instead of just the array of periods.
The new functions are as follows:

/**
 * Get the formatted schedule json for a specific day
 * @param {dayjs} date - The date to get the schedule for
 * @returns {Schedule} - The formatted schedule json
 */
export function getSchedule(date);
/**
 * Get the periods for a specific schedule type 
 * @param {string} scheduleType - The string of schedule type to get the periods for.
 * @returns {Object.<string, timeString[]>[] | "NONE"} - The periods for the schedule type
 */
function getPeriods(scheduleType);
/**
 * 
 * @param {Object.<string, timeString[]>[] | "NONE"} periodList - The periods to get the periods json for
 * @returns {Period[]} - The formatted periods json including passing periods
 */
function getPeriodsJSON(periodList);
/**
 * Get the schedule default and override types for a specific date
 * @param {dayjs} date - The date to get the schedule type for
 * @returns {{scheduleDefault: string, scheduleOverride: string}} - The schedule types for the date
 */
function getScheduleTypes(date);
/**
 * Gets the schedule type for a specific date without specifying the override
 * @param {dayjs} date - The date to get the schedule type for
 * @returns {string} - The schedule type for the date
 */
export function getScheduleType(date);

This file has also been equipped with some JSDoc comments to make typing easier.

Speaking of schedule, the schedule.json file has had some upgrades.
dateRanges.breaks is now a multidimensional array allowing for multiple date ranges for the same schedule type. This should ease the transition between one break and the next so it doesn't just disappear off the calendar when you add the next one.
gradeLevels has been split into specific grades, ie. GRADE_7, GRADE_8, ... where now you can specify individual schedules for each grade level. For grade levels that share a schedule, 7/8, there is inheritsFrom.

  "GRADE_8": {
    "inheritsFrom": "GRADE_7"
  },

Future Improvements

During this upgrade, I kept thinking about the size of the files being sent to the clients, so I was thinking we could add a minifying step to the Firebase builds so that unnecessary comments and code would be pruned and shrunk to make for a smaller file size. It is not very necessary, but it could be nice.
I also think we should totally have achievements to collect and badges when you get them so people can find easter eggs and show them off with a realtime leader board and ranks and seasons and special cosmetics.

Small Changes

Overall code has been cleaned, renamed for clarity, or removed because of not being used or not needed.
Additionally, all styles and CSS have been rewritten from scratch to remove unneeded classes or elements from the DOM. These new styles should be more cohesive, nicer looking, more accessible, and better for responsiveness on all platforms.
There is also a new setting for Emoji Rain where one can specify an emoji that will fall in the background of the page.

Lastly, the version numbers still need to be updated in the data files to reflect the new version.

@mark-crichton
Copy link
Collaborator

okay wow very impressive changes, thanks for doing this! if you could also do a pr for the extension, that'd be appreciated!

uh one problem tho:
image
(page constantly keeps reloading and has the broken screen there)

also i see you sneaking in the emoji rain... my take is that if it takes very little to no effort to maintain, i'd be fine with merging it in

@aschmitigal22
Copy link
Collaborator Author

I am pretty sure that the issue is with the localStorage json data being malformed to the expected format so it is trying to load the new ones and reload, but the version numbers are the same so it isn't applying the new ones.

I just pushed a commit that add some better error logging in the places that would be not descriptive otherwise, as well as making localStorage values for eventsJSON, scheduleJSON, and languageJSON, auto-remove themselves if they fail to parse or load.

@mark-crichton
Copy link
Collaborator

seems like the error occurs when trying to update the languageJSON (and probably other similar jsons), which doesn't get removed yet

@mark-crichton
Copy link
Collaborator

mark-crichton commented Aug 31, 2023

anyways opened this in a new instance that has no storage for localhost yet, here's some things I'd like to point out:

  • I think there's too little contrast on the on/off buttons in settings, as I couldn't tell at first if there were any settings applied yet image
  • also don't like how far away it is on the app settings page; i prefer what it looks like on the class names page: image
  • actually the contrast in general between colors seems too little, might be good to change that: image
  • edit: clicking on any day in the calendar seems to always show a day with no school, no matter the day clicked (further edit: same for current period as well)

@mark-crichton mark-crichton marked this pull request as draft August 31, 2023 20:24
@mark-crichton
Copy link
Collaborator

gonna convert this to a draft for now, some breaking issues that should be addressed

@aschmitigal22
Copy link
Collaborator Author

There I have upped the contrast in most places and added some max widths.
The problem is that the background colors are different depending on browser and monitor, so I tried to match them as best I could by comparing a bunch of devices.

Screenshot 2023-09-05 at 23 36 29

…th nested elements, 🪦 rip fancy dynamicly saturated elements
@aschmitigal22
Copy link
Collaborator Author

I think this is ready to go. As far as I can tell, this is stable and if there are any more style changes that need to be made, they can be added later,

@aschmitigal22 aschmitigal22 marked this pull request as ready for review September 16, 2023 02:38
@mark-crichton
Copy link
Collaborator

still getting a reload loop when checking out from your master then switching to your develop, so can't merge unfortunately (happens to me both on arch linux firefox and ios safari) :(
how I reproduced:

  • checkout from master and host an http server
  • go to dev tools and delete everything from local storage and reload to regenerate a clean local storage
  • stop http server, switch to develop, then start http server again
  • reload the page, it starts the reload loop

@mark-crichton
Copy link
Collaborator

mark-crichton commented Sep 16, 2023

image
looked at it with a debugger, looks like userSettings.LANGUAGE is undefined (as language exists from the old settings, yet LANGUAGE doesn't from the new settings, which doesn't get fixed upon a reload)
edit: fyi this was with that latest commit (469fd98)

@mark-crichton
Copy link
Collaborator

alright, think this is in a good enough state to merge into develop (after resolving merge conflicts of course)
would be nice to also migrate to new format of settings, but don't think it's strictly necessary
thx for your work on this!

@aschmitigal22
Copy link
Collaborator Author

Yup, I am currently trying to solve those conflicts with a pull request on my fork.
And yes the old go export button does need to be updated, but as far as I can tell it just need to all caps and underscore the existing keys.

language -> LANGUAGE
themeAnimationIntensity -> THEME_ANIMATION_INTENSITY
colorTheme -> COLOR_THEME
showAMPM -> AM_PM
inlinePeriodDetails -> INLINE_DETAILS
grade -> GRADE_LEVEL
notificationToggle -> NOTIFICATIONS_TOGGLE
twentyFourHour -> TWENTY_FOUR_HOUR
themeAnimation -> THEME_ANIMATION
sixthEnabled -> SIXTH_PERIOD
zeroEnabled -> ZERO_PERIOD
showExtraPeriods -> EXTRA_PERIODS
notificationStart -> NOTIFICATIONS_START
notificationEnd -> NOTIFICATIONS_END

@mark-crichton
Copy link
Collaborator

aight
was also referring to existing settings on the new go tho (as those get cleared if invalid)
am fine with just clearing everyone's settings if need be, but don't want to do that if others aren't okay with that

@aschmitigal22
Copy link
Collaborator Author

I get what you mean, but unless we write a conversion layer to match / guess keys to the new ones, the setting will get lost.
Plus settings should only get reset if the grade level or language is not valid, because I felt those were the most vital and should always be valid.
if (userSettings == null || !settings.GRADE_LEVEL.options.includes(userSettings.GRADE_LEVEL) || !settings.LANGUAGE.options.includes(userSettings.LANGUAGE))

@mark-crichton
Copy link
Collaborator

got it
don't think it needs to be solved really, so i'll go ahead and merge into develop once you've got the conflicts sorted out

@mark-crichton mark-crichton merged commit f8bc12c into lchsiteam:develop Sep 16, 2023
1 check passed
@aschmitigal22 aschmitigal22 deleted the develop branch September 16, 2023 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants