Skip to content
This repository was archived by the owner on Aug 30, 2018. It is now read-only.

Using the settings.logo_max_width value in styles#461

Merged
cshold merged 6 commits intoShopify:masterfrom
montalvomiguelo:echo-setting-logo_max_width
Sep 17, 2015
Merged

Using the settings.logo_max_width value in styles#461
cshold merged 6 commits intoShopify:masterfrom
montalvomiguelo:echo-setting-logo_max_width

Conversation

@montalvomiguelo
Copy link
Contributor

I added that value in the stylesheet in order to get this setting working.

screen shot 2015-08-01 at 9 29 25 pm

Address #460

@cshold
Copy link
Contributor

cshold commented Aug 4, 2015

Good catch. One adjustment that we've been using lately is to not require the px value in the text box. Seems insignificant, but turns out it's enough of a hassle to non-tech people that be worth it. Can you update this to:

{
  "type": "text",
  "id": "logo_max_width",
  "label": "Max logo width",
  "info": "Defined in pixels. Do not add the 'px' unit."
}

@montalvomiguelo
Copy link
Contributor Author

Yahh You right... I have updated the files :)

Copy link

Choose a reason for hiding this comment

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

I personally do this in the theme, so even for people including the px, it does not break:

{{ settings.logo_max_width | default: '450' | remove: 'px' }}px;

Copy link
Contributor

Choose a reason for hiding this comment

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

That is smart! @montalvomiguelo are you able to adjust that line and we can merge it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

cshold added a commit that referenced this pull request Sep 17, 2015
Using the settings.logo_max_width value in styles
@cshold cshold merged commit a3ddf4c into Shopify:master Sep 17, 2015
@montalvomiguelo montalvomiguelo deleted the echo-setting-logo_max_width branch September 26, 2015 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants