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 an icon as missing image placeholder #1760

Merged
merged 2 commits into from
Mar 9, 2017

Conversation

jhawthorn
Copy link
Contributor

This displays a nicer placeholder for representing a missing image on products or variants. It uses a font icon instead of linking to a PNG asset.

This has the advantage of being i18n friendly (the previous PNG was english), and is slightly more convenient to use technically (doesn't require sprockets resolving the image path). It also looks nicer.

Before:

After:

This displays a nicer placeholder for representing a missing image on
products or variants. It uses a font icon instead of linking to a PNG
asset.

This has the advantage of being i18n friendly (the previous PNG was
english), and is slightly more convenient to use technically (doesn't
require sprockets resolving the image path). It also looks nicer.
@jhawthorn jhawthorn force-pushed the image-placeholder branch from 122dcb3 to 79bb558 Compare March 8, 2017 19:03
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Great improvement 👌🏻

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Do we already use handlebars partials?

<img src="{{ image.mini_url }}">
{{else}}
<span class="image-placeholder mini"></span>
{{/if}}
Copy link
Member

Choose a reason for hiding this comment

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

Could this be a handlebars partial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about that because we need to do the lookup of mini_url/small_url, but maybe a helper. I'll investigate.

Copy link
Contributor Author

@jhawthorn jhawthorn Mar 9, 2017

Choose a reason for hiding this comment

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

It can be done! I think it warrants further discussion. I will open a follow-up PR.

@jhawthorn jhawthorn merged commit c23d243 into solidusio:master Mar 9, 2017
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.

2 participants