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

STRF-4373: Removing lazyload for logo in order-confirmation.html #1159

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

jfasanobc
Copy link
Contributor

@jfasanobc jfasanobc commented Jan 31, 2018

What?

The image for the order-confirmation.html was not loading, and as with the issue in STRF-4138, I've removed the lazyload for the logo for this template page.

Tickets / Documentation

Add links to any relevant tickets and documentation.

Screenshots (if appropriate)

Before

After

@bigbot
Copy link

bigbot commented Jan 31, 2018

Autotagging @bigcommerce/storefront-team @davidchin

@jfasanobc jfasanobc changed the title Removing lazyload for logo in order-confirmation.html STRF-4373: Removing lazyload for logo in order-confirmation.html Jan 31, 2018
@junedkazi
Copy link
Contributor

@mjschock just making sure you saw this PR bcoz there is a ticket assigned to you for this sprint.

@@ -16,7 +16,7 @@ <h1 class="is-srOnly">{{lang 'checkout.title'}}</h1>
<h2 class="checkoutHeader-heading">
<a class="checkoutHeader-link" href="{{urls.home}}">
{{#if checkout.header_image}}
<img alt="{{settings.store_logo.title}}" class="checkoutHeader-logo lazyload" id="logoImage" data-sizes="auto" src="{{cdn 'img/loading.svg'}}" data-src="{{ checkout.header_image }}"/>
<img alt="{{settings.store_logo.title}}" class="checkoutHeader-logo" id="logoImage" src="{{ checkout.header_image }}"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with this change but instead of removing the lazy load we should be investigating what is causing the failure to load in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I was just looking at a quick solution for now, as it's only this and another page from a previous PR. I tried researching it a bit, but it was bit over my head as to why. Thanks for reviewing, I wasn't sure as to why it wouldn't let me add reviewers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we support running theme scripts on the order confirmation page? I believe the lazysizes script just isn't being inserted there, despite being part of global.js. Assuming that's the case, this is quite possibly the correct solution, short of manually inserting a <script> tag in the order confirmation template.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing that is what is happening as well.

@mjschock
Copy link
Contributor

i'm going to go ahead a merge this - we'll create a separate ticket to pull out the shared code between the checkout and order-confirmation pages. i was thinking of something like a common checkout partial where we pass the down {{{ checkout.checkout_content }}} or {{{ checkout.order_confirmation_content }}} respectively as {{> @partial-block}}. @jfasanobc - if you want to create another PR to do that you're more than welcome but this is fine for now.

@mjschock mjschock merged commit 4bd34eb into bigcommerce:master Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants