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

Reduced dependence on snippets#358

Closed
cshold wants to merge 3 commits intomasterfrom
snippet-example
Closed

Reduced dependence on snippets#358
cshold wants to merge 3 commits intomasterfrom
snippet-example

Conversation

@cshold
Copy link
Contributor

@cshold cshold commented Mar 16, 2015

Here is an experimental PR that places all code from one-use snippets in line.

Pros of this approach:

  • Reduced noise in /snippets
  • Easier for merchants to find elements per template
    • Prime example was to find the {{ powered_by_link }}. Timber used to have it in snippets/footer, rather than layouts/theme - the only place it is ever used

Cons:

  • Makes template/layout files much longer and harder to traverse (at times)

My preference would be to take any snippet that is only used once and move it inline. Exceptions would be:

  • Open graph meta tags
  • Twitter card meta tags
  • Old IE shivs/polyfills
  • Handlebar.js ajax cart templates
  • Onboarding markup?
    • These are used once, but will never be to be edited by a merchant

These make the main layout file much longer than it needs to be, as seen in this PR. These are never going to be touched by a merchant and therefor offer no value in the main layout file.

@mpiotrowicz @carolineschnapp @fredryk @stevebosworth

@carolineschnapp
Copy link
Contributor

I am in total agreement with the general rule proposed here, and the listed of exceptions provided.

@carolineschnapp
Copy link
Contributor

Eventually we could make a case that {{ content_for_header }} already contain Open graph tags and Twitter rich snippets, since the likelihood of a merchant editing those files is very low.

@fredryk
Copy link

fredryk commented Mar 16, 2015

Ten snippets lighter! I'm all for this, and the list of exceptions.

@carolineschnapp
Copy link
Contributor

Another situation where snippets are needed now is when you have theme settings that let you populate a sidebar or a homepage with “widgets” in the order that you want.

Like we do in the Solo theme. Perhaps there's another way to handle that fine-grained control that we obtain with snippets then.

Solo's index template contains this:

{% if settings.home_section_1 != blank %}
  {% include settings.home_section_1 %}
{% endif %}

@cshold
Copy link
Contributor Author

cshold commented Mar 16, 2015

I think that's another conversation to be had for sure, but not handled in this PR. Right now it's the best solution I can think of to handle the ordering of settings like that.

@carolineschnapp
Copy link
Contributor

Sorry, not the right place for sure.

@cshold cshold mentioned this pull request Mar 17, 2015
@cshold
Copy link
Contributor Author

cshold commented Mar 17, 2015

Using #361 for this now.

@cshold cshold closed this Mar 17, 2015
@cshold cshold deleted the snippet-example branch October 5, 2015 16:35
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

Comments