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

Use largo-modernizr as dependency for citylimits-navigation #86

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

joshdarby
Copy link

Changes

This pull request makes the following changes:

  • Updates the citylimits-navigation enqueue to use largo-modernizr as a dependency to (hopefully) solve the Modernizr.hasEvent is not a function error on staging

Why

For #39

Testing/Questions

Features that this PR affects:

  • Site header

Questions that need to be answered before merging:

  • Is this PR targeting the correct branch in this repository?

Steps to test this PR:

  1. Make sure everything still works and no new errors are present

@joshdarby joshdarby requested a review from benlk October 31, 2019 20:23
@joshdarby joshdarby self-assigned this Oct 31, 2019
@joshdarby joshdarby added this to In progress in CITY-008 - Website Redesign via automation Oct 31, 2019
CITY-008 - Website Redesign automation moved this from In progress to Reviewer approved Oct 31, 2019
Copy link
Collaborator

@benlk benlk left a comment

Choose a reason for hiding this comment

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

We should do this in Largo, as well. https://github.com/INN/largo/pull/1715/files for WPBuddy/largo#1714 defined the function, but if the dependency is necessary, we should make sure it's mentioned in the enqueues.

@benlk
Copy link
Collaborator

benlk commented Oct 31, 2019

Reopened WPBuddy/largo#1714 to track that task.

@joshdarby joshdarby merged commit 961791d into master Nov 1, 2019
CITY-008 - Website Redesign automation moved this from Reviewer approved to Done Nov 1, 2019
@kaylima
Copy link
Member

kaylima commented Nov 11, 2019

@josh can you describe how the issue pertains to the logo, in particular? is there any sort of workaround we can do so I can QA that section?

@joshdarby
Copy link
Author

@kaylima Largo uses Javascript to place the src attribute on the logo image element in the header.

From what I can tell, somewhere in the new navigation js file there is an issue with a dependency (Modernizr) that is preventing the logo image from being placed in.

A quick work around would be to just take their logo image url (https://citylimits.org/wp-content/uploads/2014/10/CL_logo_768-1-1-2.png) and search for the image element in the page HTML with the header_img class in the deve console and replace src(unknown) with src="https://citylimits.org/wp-content/uploads/2014/10/CL_logo_768-1-1-2.png"

@kaylima kaylima added the Priority: High Either blocks work on a priority-normal task or a solution here informs other work. label Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Either blocks work on a priority-normal task or a solution here informs other work. Status: Review Needed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants