Skip to content

Add iframe component. #233

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 18, 2019
Merged

Add iframe component. #233

merged 2 commits into from
Nov 18, 2019

Conversation

mayanktap
Copy link
Contributor

Issue #193: Addition of Iframe component.

Changes

  • Add iframe component.
  • Add iframe docs.
  • Add iframe spec.

@mayanktap
Copy link
Contributor Author

Hi @pascalwengerter ,
This is about Iframe tags. Please review and let me know in case of any discrepancy.

@mayanktap mayanktap changed the title Add iframe component with doc and spec. Add iframe component. Oct 9, 2019
pascalwengerter
pascalwengerter suggested changes Oct 14, 2019
View reviewed changes
app/concepts/matestack/ui/core/iframe/iframe.rb
@@ -0,0 +1,5 @@
module Matestack::Ui::Core::Iframe
class Iframe < Matestack::Ui::Core::Component::Static

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a lot of the attributes an iframe tag can have (see reference). See youtube or image component for reference on how to include multiple attributes into the @tag_attributes!

@pascalwengerter
Copy link
Contributor

Hey @mayanktap, not bad! Could you include more attributes from the w3school iframe definition (using the @tag_attributes, like the youtube or image component already do)?

@mayanktap
Copy link
Contributor Author

Sure @pascalwengerter . I will take a look at this.

@pascalwengerter
Copy link
Contributor

Hey @mayanktap do you need any help to go the final steps on this one? 🙌

@mayanktap
Copy link
Contributor Author

mayanktap commented Oct 30, 2019 •
edited
Loading

Hey @pascalwengerter ,
Actually I went for a long vacation and reached few days ago. I will get back to you on this in 2-3 days.

@pascalwengerter
Copy link
Contributor

No worries @mayanktap, just let us know if you need help or decide you want to stop working on it :)

@mayanktap
Copy link
Contributor Author

Hey @pascalwengerter ,
I have made some changes based on your feedback. Please take a look on it.
And sorry for the delay.

pascalwengerter
pascalwengerter approved these changes Nov 18, 2019
View reviewed changes
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.

Looks good to me now!

@pascalwengerter pascalwengerter merged commit d35e7aa into matestack:develop Nov 18, 2019
@pascalwengerter pascalwengerter mentioned this pull request Nov 18, 2019
Closed
@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
Reviewers

@pascalwengerter pascalwengerter pascalwengerter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.7.3
Development

Successfully merging this pull request may close these issues.
3 participants
@mayanktap @pascalwengerter @jonasjabari