Allow HTML embed code for CodePen #59
Conversation
| if node["src"] == CODE_PEN_SCRIPT_URL | ||
| node.children.unlink | ||
| else | ||
| node.unlink |
There was a problem hiding this comment.
The responsibility of AttributeFilter is to filter attributes but #filter_script unlinks <script> nodes. IMO, you should add a new transformer class and move this method to it according to SRP.
| "data-pen-title", | ||
| "data-slug-hash", | ||
| "data-theme-id", | ||
| "data-user", |
There was a problem hiding this comment.
This array duplicates UserInputSanitizer::CODE_PEN_ATTRIBUTES. You should define a Qiita::Markdown::CodePen module which contains constants corresponding to codepen.
There was a problem hiding this comment.
And please remove all data-* attributes except data-slug-hash and data-embed-version.
|
@yuku-t Thank you for reviewing! I defined |
koki-sato
force-pushed
the
code-pen-html
branch
3 times, most recently
from
97368cc to
33b6aac
Compare
| context "with HTML embed code for CodePen" do | ||
| let(:markdown) do | ||
| <<-EOS.strip_heredoc | ||
| <p data-slug-hash="foo" data-embed-version="2" class="codepen"></p> |
There was a problem hiding this comment.
Since the <p> doesn't have other data-* attributes, this spec doesn't test whether data-attributes except data-slug-hash and data-embed-version are sanitized. It should have at least data-height, data-theme-id, data-default-tab, data-user and data-pen-title attributes.
|
OK, I changed the test to check whether |
| end | ||
| end | ||
|
|
||
| shared_examples_for "codepen" do |allowed:| |
There was a problem hiding this comment.
"codepen" is inappropriate as the name of the shared examples, because codepen is allowed to use even though the parameter is false. I think it should be renamed as "override codepen attributes".
| EOS | ||
| end | ||
| else | ||
| it "sanitize data-attributes except the minimum attributes" do |
There was a problem hiding this comment.
it "sanitizes ...
Please start each spec with a verb in the third person singular form.
koki-sato
force-pushed
the
code-pen-html
branch
3 times, most recently
from
c5d79b5 to
3d1cf15
Compare
yuku
left a comment
yuku
left a comment
There was a problem hiding this comment.
LGTM. @koki-sato Please squash commits
koki-sato
force-pushed
the
code-pen-html
branch
from
3d1cf15 to
da8fba3
Compare
Relates to #54
In this PR, embed CodePen by html and javascript ( the 2nd one of #54 (comment) )
Accordingly, allow some
data-*attribute for<p>and<script>for codepen's javascript.