Fix bug with AM and PM classes #268
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
psvenk
requested changes
Feb 15, 2021
There was a problem hiding this comment.
Thank you @jadebuckwalter for the changes. Instead of hard-coding "l" for AM/PM classes, I think it would be a better idea to detect regular periods (01, 02, 03, 04) and fall back to a strict equality check, similar to how it worked before 9d2d9a8. Maybe we could do a regex search on period for /0\d/ to detect a regular period, and if so, see if the same period number is a substring of the default name. Your thoughts?
|
@psvenk Sure, that makes sense. |
Detect difference between AM and PM period names so they show up in the correct places
psvenk
force-pushed
the
fix-am-pm-repeat
branch
from
f7b701f
to
372a2d1
Compare
Instead of relying solely on the last character of the period name, include explicit support for multiple period name formats so as to reduce the rate of false positives (e.g., "Before School" and "After School" being detected as the same period).
psvenk
approved these changes
Feb 17, 2021
There was a problem hiding this comment.
Looks good to me now. @jadebuckwalter, feel free to squash and merge (maybe using something like my commit message) once you have verified that this works for you.
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Detect difference between AM and PM period names so they show up in the
correct places