-
-
Notifications
You must be signed in to change notification settings - Fork 43
ISSUE #183 Adds del component #225
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cameronnorman, great work but I'm missing the cite
and datetime
params from the HTML API, details see here: https://www.w3schools.com/tags/tag_del.asp
Could you add them?
@cameronnorman I think if you merge the latest version of the |
bd74b79
to
895094f
Compare
Hey @pascalwengerter @michaelrevans please can I get a review on this one now that the branch is up to date and passing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
@@ -0,0 +1,5 @@ | |||
%del{@tag_attributes.merge({:cite => options[:cite], :datetime => options[:datetime]})} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the @tag_attributes.merge([...])
stuff to be within a setup method in the del.rb, but that's a personal preference. Should be fine like this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
Issue #183: Adds
del
componentChanges
Notes