Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Add the sections folder back to webpack context #1005

Merged

Conversation

dan-gamble
Copy link
Contributor

What are you trying to accomplish with this PR?

Fix a small issue introduced by #1000.

With the new Slate Sections Plugin introduction, the sections folder got removed from Webpack's context. I believe it was previously added there by the CopyWebpackPlugin.

This PR adds it back into the context so making changes to the files causes the browser to reload as it used to.

Please provide a link to the associated GitHub issue.
#1004

Checklist

For contributors:

@ghost ghost added the cla-needed label Mar 7, 2019
@dan-gamble
Copy link
Contributor Author

I signed the CLA but not quite sure how to re-run it.

@ghost ghost removed the cla-needed label Mar 7, 2019
@harshal317
Copy link
Contributor

Thanks a lot @dan-gamble, I made some styling changes so it would pass CI. I also got rid of the config import, as the sections path should be what we receive as the to option in the plugin, so figured it be best to just use that. I'll let @t-kelly take a look before publishing.

Copy link
Contributor

@wizardlyhel wizardlyhel left a comment

Choose a reason for hiding this comment

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

👍

@dan-gamble
Copy link
Contributor Author

Awesome. I figured there would be some changes as I'm not that au fait with the Shopify coding standard.

@treechime
Copy link

Thanks, @dan-gamble!

Can this be expected to be merged soon? The current version is very frustrating to develop with until this is fixed...

Copy link
Contributor

@t-kelly t-kelly left a comment

Choose a reason for hiding this comment

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

Should we be adding the files from this.options.from instead of this.options.to?

packages/slate-sections-plugin/index.js Outdated Show resolved Hide resolved
packages/slate-sections-plugin/index.js Outdated Show resolved Hide resolved
@harshal317 harshal317 requested a review from t-kelly March 11, 2019 15:17
@harshal317
Copy link
Contributor

Thanks, @dan-gamble!

Can this be expected to be merged soon? The current version is very frustrating to develop with until this is fixed...

Sorry for the wait! Ideally we should get this merged today.

@harshal317
Copy link
Contributor

Should we be adding the files from this.options.from instead of this.options.to?

That was a mistake, ended up just adding to the context when we tap the emit hook, and added making sure the sections folder is in the context to the tests. 🎩 locally worked.

@harshal317 harshal317 merged commit 4e93c0a into Shopify:master Mar 11, 2019
@harshal317 harshal317 added the bug label Mar 11, 2019
@treechime
Copy link

Perfect, thanks all 👍

@lock
Copy link

lock bot commented Apr 11, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants