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

Fix period numbering under clock #242

Merged
merged 4 commits into from
Jan 6, 2021

Conversation

jadebuckwalter
Copy link
Collaborator

Write a conditional to check whether a schedule includes a "before
school" class, as well as to align the black and silver day schedules.

Write a conditional to check whether a schedule includes a "before
school" class, as well as to align the black and silver day schedules.
@jadebuckwalter jadebuckwalter linked an issue Jan 6, 2021 that may be closed by this pull request
Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work with my schedule, but it seems a bit hacky. Instead of computing an index using the period number (Number(default_name.charAt(default_name.length - 1))), we could just compare the last character of default_name with the last character of each entry of period_names[bs_day] (for (const { name } of period_names[bs_day]) { ... }, for example). If there is a match, then we can assume that this is the correct period and return the name; if no match, then just return the default_name. This way, we don't have to special-case "Before School" (AM) classes, we don't need to assume that they all have the words "Before School" in the title, we don't have separate logic for black and silver days, and the code is overall a lot simpler. Your thoughts?

@jadebuckwalter
Copy link
Collaborator Author

@psvenk Yes, that's a better idea.

Fix period numbering under small clock by using the current period
rather than detecting AM classes.
@jadebuckwalter jadebuckwalter requested a review from psvenk January 6, 2021 22:02
@jadebuckwalter
Copy link
Collaborator Author

@psvenk This should work, and it doesn't rely on detecting before school classes.

psvenk added 2 commits January 6, 2021 17:52
Remove the superfluous "Period: " before period names; remove
special-casing for "Community Meeting" (which anyway is not enforced due
to the change to "CM/ADV"); simplify get_period_name; fix indentation
rule for clock.js in .editorconfig
Change some code so that it does not assume that the periods are the
same for black days as for silver days.
Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jadebuckwalter I made a few changes; do you think this is ready to merge now?

@psvenk psvenk added this to the 2.7.0 milestone Jan 6, 2021
@jadebuckwalter jadebuckwalter merged commit 5a0de6a into Aspine:master Jan 6, 2021
@jadebuckwalter jadebuckwalter deleted the period-numbering branch January 6, 2021 23:02
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.

Period numbering incorrect in clock
2 participants