Rename things called "room" to "location" to reflect current data#1687
Open
Purplegaze wants to merge 2 commits intoCourseography:masterfrom
Open
Rename things called "room" to "location" to reflect current data#1687Purplegaze wants to merge 2 commits intoCourseography:masterfrom
Purplegaze wants to merge 2 commits intoCourseography:masterfrom
Conversation
Pull Request Test Coverage Report for Build 32b1a43c-35a3-400a-8d58-85400bbeaabbDetails
💛 - Coveralls |
Contributor
david-yz-liu
left a comment
There was a problem hiding this comment.
Overall the changes are good, I just left one comment clarifying the scope of this PR.
| meeting MeetingId | ||
| firstRoom T.Text Maybe | ||
| secondRoom T.Text Maybe | ||
| firstLocation T.Text Maybe |
Contributor
There was a problem hiding this comment.
You should not make changes directly to the table schema itself without making a migration. I think that's a bit larger of a task, so for now please revert changes to the Times data type (but you can keep changes to the Time and Time' data types).
7 tasks
This file contains hidden or 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
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.
Proposed Changes
Renames most instances of things called "room" to use the word "location" instead. This better reflects the current data stored in the timetable, which no longer includes room numbers. I think "location" makes sense as a word here over something like "building", because a meeting time is not necessarily situated inside a building (e.g. online synchronous courses.)
Things that were changed:
Times,Time', andTimedatatypesfirstRoom,fstRoom, etc.)getRoomStrinreact_modal.jsThings that have NOT been changed:
Time'datatypeassignedRoom2, which does not exist in the timetable JSON response anymore), and may be connected to a bug where year-long courses are not included in the database. This definitely seems like something that warrants its own PR, and I need to learn more about the database to explore this further.roomfield. I'm not sure what feature of the website this is as I can't find any working maps in Courseography, so I didn't touch it (aside from where it used the aforementioned datatypes that I renamed)...
Screenshots of your changes (if applicable)
The only visual change is that this column in the course modal now says "Location" instead of "Room".Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
Would like some clarification on what the MapModal is and if I should be trying to fix it somehow.