-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
Display VRMS data on Events and Project Meetings pages #3833
Display VRMS data on Events and Project Meetings pages #3833
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Review ETA: End of Day 1/17/23 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi jyaymie – Looks good- the /events
and /project-meetings
pages both seem to be displaying the meeting times correctly, and the individual project pages do not seem to break. I started a tedious exercise comparing the before and after vrms_data.json
files. As Matt already pointed out, there are a couple dozen lines of code that were deleted in ‘testy test Team Meeting’ that were previously included in project:
and now are null, which may have been the breakpoint.
I am marking my review as ‘approved’ - I have nothing to add to Matt’s or your insights, but I will be following along to see how the two of you resolve this.
Great job moving this forward.
Moving this bug discovery log out of "resolved conversation" for future reference: After console.logging the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyaymie nice work knocking down a large
time-sensitive
issue so quickly! 💪
Appreciate your explanation of the proposed changes and the quality screenshots.
My only tiny suggestion is to use the check boxes provided under the issue action items, but I have seen lots of other developers here not using them so maybe I just have a weird thing for check marks 🤔
Good call, @MattPereira. Those boxes are there for a reason! |
Fixes #3701
What changes did you make and why did you make them?
insertEventSchedule
anddisplay_object
functions inassets/js/utility/api-events.js
, which fixed TypeErrors caused by incompatible or missing values in the data. Rather than combing through the VRMS data to find what caused these errors, I thought this was a more efficient solution that safeguards against similar issues in the future.Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied