Skip to content

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

Merged
merged 7 commits into from
Nov 19, 2019
Merged

ISSUE #183 Adds del component #225

merged 7 commits into from
Nov 19, 2019

Conversation

cameronnorman
Copy link
Collaborator

@cameronnorman cameronnorman commented Oct 4, 2019

Issue #183: Adds del component

Changes

  • Adds the the del component to main components

Notes

  • Nothing special here

@cameronnorman cameronnorman changed the title ISSUE #183 ISSUE #183 Adds del component Oct 4, 2019
@cameronnorman cameronnorman changed the base branch from master to develop October 4, 2019 14:39
Copy link
Contributor

@pascalwengerter pascalwengerter left a 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?

@pascalwengerter
Copy link
Contributor

@cameronnorman I think if you merge the latest version of the develop branch into your issue-183 branch the CI won't complain anymore!

michaelrevans
michaelrevans previously approved these changes Nov 7, 2019
@cameronnorman
Copy link
Collaborator Author

Hey @pascalwengerter @michaelrevans please can I get a review on this one now that the branch is up to date and passing

Copy link
Contributor

@pascalwengerter pascalwengerter left a 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]})}
Copy link
Contributor

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.

Copy link
Collaborator

@michaelrevans michaelrevans left a comment

Choose a reason for hiding this comment

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

🚢

@pascalwengerter pascalwengerter merged commit 3ae4dac into develop Nov 19, 2019
@pascalwengerter pascalwengerter deleted the issue-183 branch November 19, 2019 09:51
@jonasjabari jonasjabari added this to the 0.7.3 milestone Feb 10, 2020
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.

4 participants