Shopify / slate

Slate is a toolkit for developing Shopify themes. It's designed to assist your workflow and speed up the process of developing, testing, and deploying themes.
https://shopify.github.io/slate
MIT License
1.28k stars 364 forks source link

[WIP] Slate Sections Plugin #982

Closed harshal317 closed 5 years ago

harshal317 commented 5 years ago

What are you trying to accomplish with this PR?

In order to support a sections format that allows separating the schema from the liquid, and allowing a structure that makes it easy to maintain translations for many languages, @t-kelly and I came to a conclusion it may be a good idea to move away from CopyWebpackPlugin. Especially keeping in mind the potential for additional features to this plugin.

This plugin aims to support three basic structures 1) Having a sections folder consisting of simply liquid files, which will simply be copied to the dist sections path

2) Having a sections folder consisting of folders for each section following the naming convention /sections/example-section-name/ having a mandatory 'template.liquid' file, and an optional 'schema.json' file. If there is a schema file in the directory it will get appended to the liquid file adding the schema tags, and will be outputted to the dist sections path taking the name of the directory (example-section-name.liquid)

3) Similiar to option 2, however it extends the functionality by adding a locales folder which can have multiple json files for each language. The translations will automatically be added to the json object that gets appended to the liquid making it easier to maintain multiple languages and keeping a cleaner codebase

To Do

Checklist

For contributors:

For maintainers:

harshal317 commented 5 years ago

The dirent class was added in Node 10, so might have to change that logic slightly to support older versions.

t-kelly commented 5 years ago

From @davidwarrington:

Would it be possible to add .js file support to this? Being able to import a schema exported from a JS file would open up a lot of possibilities. For example; querying an API to retrieve a list of options, or simply being able to build multiple similar options with a for loop. Most stores probably don't require anything like this however I have worked on a few stores where this would be beneficial.

The way I see this working is by using const jsonSchema =require(jsonSchemaFilePath) in line 22 of inject-schemas-to-sections.js. Since Node caches modules, they would need to be flushed out of the cache when updating, in order to be re-imported, but that's not a particularly difficult job.

harshal317 commented 5 years ago

From @davidwarrington:

Would it be possible to add .js file support to this? Being able to import a schema exported from a JS file would open up a lot of possibilities. For example; querying an API to retrieve a list of options, or simply being able to build multiple similar options with a for loop. Most stores probably don't require anything like this however I have worked on a few stores where this would be beneficial.

The way I see this working is by using const jsonSchema =require(jsonSchemaFilePath) in line 22 of inject-schemas-to-sections.js. Since Node caches modules, they would need to be flushed out of the cache when updating, in order to be re-imported, but that's not a particularly difficult job.

This is definitely something that can be done, however it is probably worth discussing whether we want to do it in this current PR or as a feature moving forward.

dan-gamble commented 5 years ago

This is turning in to an awesome PR 👏

Can't wait to start using this on projects

dan-gamble commented 5 years ago

Since folders are looking to be introduced here would it be much extra effort to allow an additional layer of folders in the sections folder? For example:

./sections
├── homepage/
   ├── featured-collections/
      ├── schema.json
      ├── template.liquid
   ├── hero/
      ├── schema.json
      ├── template.liquid
   ├── ...
├── product/
   ├── featured-articles/
      ├── schema.json
      ├── template.liquid
   ├── variants/
      ├── schema.json
      ├── template.liquid
   ├── ...
harshal317 commented 5 years ago

This looks great! @t-kelly made my life easier with his early reviews (thanks). I still need to 🎩but I'll do that first thing tomorrow morning.

Side note: did you and @t-kelly discuss the possibility of migrating the locale logic over so we can use it for merchant-facing translations for config/settings_schema.json, not just for sections? This would enable a similar folder structure to how we're structuring section folders and enable translations to be referencing with the t key:

./config
├── settings_schema.json
├── locales/
   ├── en.json
   ├── fr.json
   ├── ...

We haven't discussed that yet, however if it would be helpful to do so, I don't see why not. Could probably consider doing that through the CopyWebpackPlugin's transform as well.

harshal317 commented 5 years ago

Since folders are looking to be introduced here would it be much extra effort to allow an additional layer of folders in the sections folder? For example:

./sections
├── homepage/
   ├── featured-collections/
      ├── schema.json
      ├── template.liquid
   ├── hero/
      ├── schema.json
      ├── template.liquid
   ├── ...
├── product/
   ├── featured-articles/
      ├── schema.json
      ├── template.liquid
   ├── variants/
      ├── schema.json
      ├── template.liquid
   ├── ...

I believe we discussed having a deeper folder structure, and decided that it could be added as a feature moving forward.

bertiful commented 5 years ago

We haven't discussed that yet, however if it would be helpful to do so, I don't see why not.

It's definitely helpful, and it reflects how we're already approaching merchant-facing translations (this is how locales are currently formatted in our themes for both the ./sections and ./config folders).

dan-gamble commented 5 years ago

Didn't think this would be in so quick. Can't wait for 0.16 😍

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.