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

feat(gatsby-remark-code-repls): include matching css option (CodePen) #12110

Conversation

deblasis
Copy link
Contributor

@deblasis deblasis commented Feb 26, 2019

Description

Currently it's possible to upload multiple files only by using CodeSandbox.

This PR adds an option that allows adding CSS files to CodePen examples by setting the css property in the payload object.

Ref: https://blog.codepen.io/documentation/api/prefill/

Matching CSS means that if the user is linking to example.js and the option is set to true, if example.css is present in the same directory, its content is sent to CodePen.

  • The default behaviour is left untouched
  • If the option is set to true and the matched file doesn't exist, nothing changes
  • tests included

Related Issues

options: {
....
// Include CSS with matching name.
// This option only applies to REPLs that support it (eg Codepen).
// If set to `true`, when specifying `file1.js` as example file,
// it will try to inject the CSS in `file1.css` if the file exists,
// otherwise the default behaviour is preserved
includeMatchingCSS: false,
}
@deblasis deblasis changed the title Package gatsby-remark-code-repls feature: include matching css option (CodePen) feat(gatsby-remark-code-repls): include matching css option (CodePen) Feb 28, 2019
@wardpeet
Copy link
Contributor

i'm not sure this is the best API. Maybe we should enable custom config per provider?

// In your gatsby-config.js
{
  resolve: 'gatsby-remark-code-repls',
  options: {
    codepen: {
      // specific options
    }
  }
}

@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Mar 19, 2019
confict resolved

Signed-off-by: Alessandro De Blasis <[email protected]>
@deblasis
Copy link
Contributor Author

Hi @wardpeet, I thought about it but there are other options that are provider-specific and they are currently inside options:

that's why I followed the same pattern.

If I moved only the new option includeMatchingCSS inside options.codepen, the API would definitely be confusing.

The best thing would be to refactor all the options and to scope them depending on the provider that supports them but that would break backwards compatibility unless we put in place fallbacks.

The scope of this PR is to add the feature in a non-breaking fashion.

If you feel like that the API should change, I reckon it's a separate refactoring issue that should also cover all the test cases, update the README.md, etc.

Thoughts?

Signed-off-by: Alessandro De Blasis <[email protected]>
Signed-off-by: Alessandro De Blasis <[email protected]>
Signed-off-by: Alessandro De Blasis <[email protected]>
deblasis added a commit to deblasis/gatsby that referenced this pull request Mar 29, 2019
…ring

This is based upon suggestions in PR gatsbyjs#12110 review.
Since there is a breaking change in the API, the version has been ⬆

Signed-off-by: Alessandro De Blasis <[email protected]>
@deblasis
Copy link
Contributor Author

Hi @wardpeet, I have created a new PR #12936 which is mutually exclusive with this one.
#12936 implements the refactored API for the provider-specific options as discussed above.
#12110, instead, has just the non-breaking changes and the new feature.

@wardpeet wardpeet self-assigned this Apr 11, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Make sense, thanks a bunch i'll test this one out.

@@ -1,7 +1,7 @@
{
"name": "gatsby-remark-code-repls",
"description": "Gatsby plugin to auto-generate links to popular REPLs like Babel and Codepen",
"version": "2.0.8",
"version": "2.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to update package.json yourself.

Suggested change
"version": "2.1.0",
"version": "2.0.8",

@wardpeet
Copy link
Contributor

I've tested this and it works how it should. Thanks for the effort

@wardpeet wardpeet added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: awaiting author response Additional information has been requested from the author labels Apr 12, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Thanks a bunch!

@wardpeet wardpeet merged commit 1b8d93e into gatsbyjs:master Apr 12, 2019
@gatsbot
Copy link

gatsbot bot commented Apr 12, 2019

Holy buckets, @deblasis — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

deblasis added a commit to deblasis/gatsby that referenced this pull request Apr 27, 2019
Apparently we merged to master the previous version that added the feature to include the matching
css in the folder if present. This version is the refactored one which also improves the API surface
by scoping the options depending on the REPL provider

BREAKING CHANGE: The API surface is not backward compatible anymore as the options have been scoped
per-provider as suggested during code-review gatsbyjs#12110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants