Hours #92
Hours #92
Conversation
There was a problem hiding this comment.
I see that this resolves most of the hours locations, with two exceptions - the homepages for the Docs and Distinctive Collections sites are still not loading hours. This is a solvable problem, but I'm not 100% clear on what the best way forward is without spending more time on this than a review.
I've also got some questions about three of the choices you've made here - not in the sense of "this won't do, please change" but more of "what do you think about an alternative approach".
I think I've raised the questions clearly via inline comments, but just to be sure, they are:
- Would it be better to debug why the existing widgets aren't registering/enqueuing the hours load script as is, or to take the route you're taking here and just handle it via the theme layer?
- Would you prefer to handle the hours information on the location template via this approach, or via defining the location hours sidebar? AFAICT, the two approaches are an either/or proposition (the sidebar's only purpose is to do what you're doing here in code)
- Similar to the first question, enqueuing the hours loader directly in the theme will resolve the hours page template, but there's an alternative approach (go all in on the hours plugin) - but that will need engineering work that was never done in the legacy theme. Is it better to just handle this via the theme?
| wp_register_script( 'parent-hours', get_template_directory_uri() . '/js/make.datepicker.js', array( 'jquery', 'gldatepickerJS', 'hours-scrollStick', 'hours-stickyMenu' ), $theme_version, true ); | ||
| wp_register_script( 'moment', get_template_directory_uri() . '/js/libs/moment.min.js', array( ), $theme_version, true ); | ||
| wp_register_script( 'underscore', get_template_directory_uri() . '/js/libs/underscore.min.js', array( ), $theme_version, true ); | ||
| wp_register_script( 'hours-loader', get_template_directory_uri() . '../../../plugins/mitlib-pull-hours/js/hours-loader.js', array( ), $theme_version, true ); |
There was a problem hiding this comment.
This makes me nervous, but I'm not sure there's a way around this right now. Typically a theme shouldn't try to register and load libraries from a plugin in this way - it should be up to the plugin to do that itself. For the widgets we've already defined, that should be happening successfully via something like https://github.com/MITLibraries/mitlib-wp-network/blob/master/web/app/plugins/mitlib-pull-hours/src/class-display-widget-frontpage.php#L109-L113
The issue in this ticket, though, is that the parent theme's page template for the hours grid is left out by this approach. That grid is defined by the theme, rather than the hours plugin, which means that the hours plugin never gets asked to load content.
I can think of two responses to this:
- Do what you're doing here, and register/enqueue the hours loader via the parent theme
- Refactor the hours page template in such a way that it gets provided by the hours plugin, and the hours loader is thus registered/enqueued there. That would likely be a significant expansion of the hours plugin, though, either because it would end up defining a fourth class for this widget type, or the plugin would define the page template (which is likely doable, but feels code-smelly)
When I was originally thinking of this ticket, I was going to explore the second path - but as I type all this out and think about our timeline, I think this might be the better option. I'm mentioning this choice here to get your thoughts, though.
There was a problem hiding this comment.
yeah no more refactors quite yet :) This is messy but honestly doesn't feel significantly messier (to me!) than a lot of the other enqueuing that is going on. In fact, hiding that in the plugin may be worse in terms of wtf is going on. I'm not sure. For now I think I'd lean towards going with this but being very open for future refactors when we have more time to think about it.
| <?php if ( true === $study24 ) : ?> | ||
| | <a class="study-24-7" href="<?php echo esc_url( $gStudy24Url ); ?>" alt="This location contains one or more study spaces available 24 hours a day, seven days a week. Click the link for more info." title="Study 24/7">Study 24/7</a> | ||
| <?php endif; ?> | ||
| <?php if ( 'hayden-library' === $slug ) : ?> |
There was a problem hiding this comment.
This works, so on one hand I'm happy to ship it - but it is the sort of mixing of content and code that we've been trying to move away from. The Location Hours sidebar was intended to be a way to get away from this, allowing UX to place widgets of the type "Location Hours - Slim" and specify which locations should be displayed. The legacy implementation is here:
https://github.com/MITLibraries/MITlibraries-parent/blob/main/content-location-2021.php#L95-L111
The "else" side of that comparison is what normally gets executed, using the page slug as the key to determine which location's hours are displayed. For the Hayden location, UX defines two widgets - one for Hayden itself, and another for the Cafe.
If this all feels too complicated, and you'd rather stick with this, I'm happy to hear that argument. In that case, though, we don't need the Location Hours sidebar to be defined.
There was a problem hiding this comment.
Interesting. I missed that the commit that added this logic was removed the next week in legacy with the widgetized solution.
Is the outcome that this already worked fine and UX needed to update widget content to resolve the problem they reported with the cafe hours not showing up for the Hayden location? I'm fine with removing this code if we don't need it, but it seems like there may be confusion about how this works we may want to check in on.
| <?php if ( true === $study24 ) : ?> | ||
| | <a class="study-24-7" href="<?php echo esc_url( $gStudy24Url ); ?>" alt="This location contains one or more study spaces available 24 hours a day, seven days a week. Click the link for more info." title="Study 24/7">Study 24/7</a> | ||
| <?php endif; ?> | ||
| <?php if ( 'hayden-library' === $slug ) : ?> |
There was a problem hiding this comment.
Same comment applies here as applies to content-location-2021.php...
There was a problem hiding this comment.
I think this can merge, but want to make sure I'm describing what I see accurately - because I think another step might be needed (maybe not as part of this PR).
With this change, I see hours in the following locations:
- The network homepage (thanks to line 215)
- The hours template (thanks to the existing line 239-242)
- The locations templates (thanks to lines 244-246)
The two sidebars being added here are working as intended - I can put content below the hours grid, and can place the Hayden & Cafe widgets on that page.
All this resolves the parent theme's support for library hours.
The part that I can't make work quite yet are to get hours displaying on two locations rendered by the Child theme:
- The Distinctive Collections site (via a widget that shows in the sidebar on most pages there)
- The Doc Services site (via a hard-coded call in the masthead, can't remember what template does it)
Both of those locations are showing errors in the console because underscore isn't available. I've been able to resolve it locally by using a different name value thanhours-loaderon lines 186 and 187, at least for Distinctive Collections, but the Docs masthead might need a bit more work.
At any rate, I'm also comfortable with this merging now and tackling the child theme issue separately.
JPrevost
force-pushed
the
hours
branch
2 times, most recently
from
16a2a73 to
31ee802
Compare
|
@matt-bernhardt I've squashed my initial mess and resolved the code climate suggestions. I'm going to squash those now and then merge this as it resolves the parent theme work for hours. I'll continue working under the same ticket for the child theme, but since I"m not sure that will complete this sprint I want to have a bit of closure on at least a portion of this work :) |
https://mitlibraries.atlassian.net/browse/LM-191 This mostly adjusts which scripts or styles are loaded for certain pages. It also reintroduces two widget areas. Note: in addition to this code, the Google Sheets API key was updated to allow requests from our pantheon network tiers.
Updates secret path to reflect upcoming changes.
2 participants
This resolves all but one of the Hours related issues in https://mitlibraries.atlassian.net/browse/LM-191
Remaining is why the Nexus location shows up at the bottom of the grid. I felt it better to move forward with review on this partial solution and get as much functional as possible and loop back for that last issue.
Developer
Secrets
Documentation
Accessibility
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Stakeholder approval
Dependencies
YES | NO dependencies are updated
Code Reviewer
(not just this pull request message)