gatsby-remark-embedder icon indicating copy to clipboard operation
gatsby-remark-embedder copied to clipboard

feat(CodePen): Add support for height & width options

Open talohana opened this issue 5 years ago • 5 comments

What: Added option to override hardcoded CodePen dimensions, with option to merge provided options with default options

Why: In order to change hardcoded CodePen's iframe width/height

How:

Each transformer can optionally export a const defaultOptions. When the plugin runs it merges provided options ( services[name]) if exist with defaultOptions with precedence of provided options, and passes them as a second argument to the transformer exported getHTML function

Checklist:

  • [x] Documentation
  • [x] Tests
  • [x] Ready to be merged

talohana avatar Oct 15 '20 14:10 talohana

Codecov Report

Merging #146 (4d96646) into main (668d719) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #146   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines          300       300           
  Branches        95        96    +1     
=========================================
  Hits           300       300           
Impacted Files Coverage Δ
src/transformers/CodePen.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 668d719...4d96646. Read the comment docs.

codecov[bot] avatar Oct 15 '20 14:10 codecov[bot]

Hi @talohana! 👋

Thanks for taking the time to implement this feature. 👊

I would however take a look at how the Twitch options are implemented and make them have a default value. Exposing a defaultOptions value to merge them with the possible given options and pass that object back into the getHTML function is adding unnecessary imo.

I see, it does look simpler, should I make the changes in this PR or create a new one?

talohana avatar Oct 16 '20 18:10 talohana

should I make the changes in this PR or create a new one?

@talohana You can just hard reset this branch onto upstream/master and push new commits

MichaelDeBoey avatar Oct 16 '20 19:10 MichaelDeBoey

will this pull request be merged soon?

hazem3500 avatar Feb 20 '21 10:02 hazem3500

will this pull request be merged soon?

I can work on it later today, @MichaelDeBoey any changes required rather than resolving the conflicts?

talohana avatar Feb 20 '21 14:02 talohana