Skip to content

Allow HTML embed code for CodePen #59

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

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Allow HTML embed code for CodePen #59

merged 1 commit into from
Nov 21, 2017

Conversation

koki-sato
Copy link
Contributor

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.

if node["src"] == CODE_PEN_SCRIPT_URL
node.children.unlink
else
node.unlink
Copy link
Contributor

Choose a reason for hiding this comment

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

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

This array duplicates UserInputSanitizer::CODE_PEN_ATTRIBUTES. You should define a Qiita::Markdown::CodePen module which contains constants corresponding to codepen.

Copy link
Contributor

Choose a reason for hiding this comment

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

And please remove all data-* attributes except data-slug-hash and data-embed-version.

@koki-sato
Copy link
Contributor Author

koki-sato commented Nov 17, 2017

@yuku-t Thank you for reviewing!

I defined Qiita::Markdown::CodePen module and moved all constants and transformer related to codepen into it.

@koki-sato koki-sato force-pushed the code-pen-html branch 3 times, most recently from 97368cc to 33b6aac Compare November 17, 2017 09:26
yuku
yuku previously approved these changes Nov 17, 2017
@yuku yuku dismissed their stale review November 17, 2017 13:03

need more specs

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>
Copy link
Contributor

@yuku yuku Nov 17, 2017

Choose a reason for hiding this comment

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

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.

@koki-sato
Copy link
Contributor Author

OK, I changed the test to check whether data-* attributes except data-slug-hash and data-embed-version are sanitized.

@@ -1268,6 +1312,33 @@
end
end

shared_examples_for "codepen" do |allowed:|
Copy link
Contributor

Choose a reason for hiding this comment

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

"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
Copy link
Contributor

Choose a reason for hiding this comment

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

it "sanitizes ...

Please start each spec with a verb in the third person singular form.

@koki-sato koki-sato force-pushed the code-pen-html branch 3 times, most recently from c5d79b5 to 3d1cf15 Compare November 20, 2017 08:41
Copy link
Contributor

@yuku yuku left a comment

Choose a reason for hiding this comment

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

LGTM. @koki-sato Please squash commits

@yuku yuku merged commit f1e1e90 into master Nov 21, 2017
@yuku yuku deleted the code-pen-html branch November 21, 2017 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants