Fix/iframe attributes #269

nguyenj · 2016-12-12T00:04:47Z

This PR addresses the issue that other data-featherlight-iframe-... attributes do not get added to the iframe when it's constructed because we're limited to just CSS attributes here.

marcandre · 2016-12-12T15:47:07Z

Looks like good work 👍

I added a few comments. Also, please don't change release, I will take care of that.

marcandre · 2016-12-12T15:39:04Z

src/featherlight.js

+					'sandbox', 'src', 'srcdoc', 'width'],
+				attrs = {};
+
+			whiteList.map(function(item) {


Why map? The return is unused...

marcandre · 2016-12-12T15:39:20Z

src/featherlight.js

 	};

+function parseAttrs(obj, prefix) {


Please use correct indent

nguyenj · 2016-12-12T15:58:34Z

@marcandre thanks for the review. I've updated the code with your comments.

marcandre · 2016-12-12T16:05:38Z

Forgot, but there should be a test modified or added that checks this too...

nguyenj · 2016-12-12T16:16:44Z

I added two tests, LMK.

marcandre

Good first test. Second one needs work

marcandre · 2016-12-12T16:50:26Z

test/featherlight_test.js

+						'data-featherlight-iframe-invalid="foo">').featherlight().click();
+
+					expect($('.featherlight iframe')).to.have.attr('src').equal('http://www.apple.com');
+					expect($('.featherlight iframe')).to.not.have.attr('allowfullscreen').equal('foo');


Did you mean attr('invalid') instead of allowfullscreen? Otherwise the test makes no sense to me.
A shorter alternative would be to add this "not" test to the iframe css test.

I didn't add it to the CSS test because jQuery's .css should take care of invalid css attributes.

marcandre

If you really want to make things even nicer, it would be better to call parseAttrs once for both css and attributes, then split the result into attributes and css (would be nice but not necessary)

marcandre · 2016-12-12T16:52:17Z

src/featherlight.js

@@ -66,31 +66,31 @@
 					'sandbox', 'src', 'srcdoc', 'width'],
 				attrs = {};

-			whiteList.map(function(item) {
+			$.each(whiteList, function(index, item) {
 				var attrValue = parseAttrs(obj, 'iframe')[item];

 				if (attrValue) {
 					attrs[item] = attrValue;
 					return attrs[item];


Useless return.

marcandre · 2016-12-12T16:53:55Z

src/featherlight.js

@@ -66,31 +66,31 @@
 					'sandbox', 'src', 'srcdoc', 'width'],
 				attrs = {};

-			whiteList.map(function(item) {
+			$.each(whiteList, function(index, item) {
 				var attrValue = parseAttrs(obj, 'iframe')[item];


Just noticed, but would be better to call parseAttrs once instead of once per item in whiteList...

nguyenj · 2016-12-12T16:59:26Z

I thought about doing that; but I wanted to leave the parseAttrs function generic in hopes for it to be reusable in other scenarios.

marcandre · 2016-12-12T17:54:13Z

You could leave parseAttrs as is and then split to result in two, no?

marcandre · 2016-12-12T17:56:14Z

What you can add to the css test is that css values are not also added as attributes. I agree that you can’t really test that normal attributes are not added to the css

…

On Mon, Dec 12, 2016 at 11:58 AM, John ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/featherlight_test.js <#269>: > + + describe('generates an iframe with tag attributes', function() { + it('adds `allowfullescreen` attribute', function() { + $('<a data-featherlight-iframe="http://www.apple.com" '+ + 'data-featherlight-iframe-allowfullscreen="true">').featherlight().click(); + + expect($('.featherlight iframe')).to.have.attr('src').equal('http://www.apple.com'); + expect($('.featherlight iframe')).to.have.attr('allowfullscreen').equal('true'); + }); + + it('does not add invalid attributes', function() { + $('<a data-featherlight-iframe="http://www.apple.com" '+ + 'data-featherlight-iframe-invalid="foo">').featherlight().click(); + + expect($('.featherlight iframe')).to.have.attr('src').equal('http://www.apple.com'); + expect($('.featherlight iframe')).to.not.have.attr('allowfullscreen').equal('foo'); I didn't add it to the CSS test because jQuery's .css should take care of invalid css attributes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#269>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACD6nxyLyC-q1lDcw0i9xyMrgspeeyHks5rHX0lgaJpZM4LKFYf> .

nguyenj · 2016-12-12T17:57:29Z

Ah, okay I understand. I'll add those a test for that.

nguyenj · 2016-12-12T18:06:00Z

I was thinking about refactoring readElementConfig to utilize parseAttrs; but I haven't gotten to chance to dive into it thoroughly yet; hence my hesitation on making it return an object with css or attr keys.

marcandre · 2016-12-12T18:12:23Z

Don’t sweat it too much. As long as you call `parseAttrs` no more than twice, it’s fine :-)

…

On Mon, Dec 12, 2016 at 1:06 PM, John ***@***.***> wrote: I was thinking about refactoring readElementConfig to utilize parseAttrs; but I haven't gotten to chance to dive into it thoroughly yet; hence my hesitation on making it return an object with css or attr keys. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#269 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACD6hfj175_vbQGpXcTsjD54aHdrLIXks5rHY0JgaJpZM4LKFYf> .

marcandre · 2016-12-22T21:03:54Z

Merged, refactored a bit. Good work, thanks!

Released in 1.7.0

nguyenj mentioned this pull request Dec 12, 2016

How to add allowfullscreen to generated vimeo iframe? #247

Closed

nguyenj force-pushed the fix/iframe-attributes branch from 19e8e83 to e12df42 Compare December 12, 2016 00:21

marcandre requested changes Dec 12, 2016

View reviewed changes

nguyenj added 4 commits December 12, 2016 07:57

Separate CSS attrs and tag attrs for iframe

7149d71

Update example on index.html file

219ebb3

Add .editorconfig

8304a4d

Add build and test to package.json scripts

a954df4

nguyenj force-pushed the fix/iframe-attributes branch from a11f783 to da6e349 Compare December 12, 2016 15:58

nguyenj force-pushed the fix/iframe-attributes branch from da6e349 to 3e0e08a Compare December 12, 2016 16:00

Fix indentation, use jQuery.each instead of map

ef143e8

nguyenj force-pushed the fix/iframe-attributes branch from 3e0e08a to ef143e8 Compare December 12, 2016 16:05

marcandre requested changes Dec 12, 2016

View reviewed changes

nguyenj force-pushed the fix/iframe-attributes branch from 64dc393 to 56e9ed8 Compare December 12, 2016 16:57

Add tests for iframe attributes

17e1253

nguyenj force-pushed the fix/iframe-attributes branch from 56e9ed8 to 17e1253 Compare December 12, 2016 18:08

marcandre closed this Dec 22, 2016

nguyenj deleted the fix/iframe-attributes branch December 23, 2016 22:50

dbworth mentioned this pull request Mar 31, 2018

Fix 3 issues (Unnecessary scrollbar on iFrames, Missing attributes for iFrame, Update readme with examples of videos in iFrame) #353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/iframe attributes #269

Fix/iframe attributes #269

nguyenj commented Dec 12, 2016 •

edited

Loading

marcandre commented Dec 12, 2016

marcandre Dec 12, 2016

marcandre Dec 12, 2016

nguyenj commented Dec 12, 2016 •

edited

Loading

marcandre commented Dec 12, 2016

nguyenj commented Dec 12, 2016 •

edited

Loading

marcandre left a comment

marcandre Dec 12, 2016

nguyenj Dec 12, 2016

marcandre left a comment

marcandre Dec 12, 2016

marcandre Dec 12, 2016

nguyenj commented Dec 12, 2016 •

edited

Loading

marcandre commented Dec 12, 2016

marcandre commented Dec 12, 2016 via email

nguyenj commented Dec 12, 2016

nguyenj commented Dec 12, 2016

marcandre commented Dec 12, 2016 via email

marcandre commented Dec 22, 2016

Fix/iframe attributes #269

Fix/iframe attributes #269

Conversation

nguyenj commented Dec 12, 2016 • edited Loading

marcandre commented Dec 12, 2016

marcandre Dec 12, 2016

Choose a reason for hiding this comment

marcandre Dec 12, 2016

Choose a reason for hiding this comment

nguyenj commented Dec 12, 2016 • edited Loading

marcandre commented Dec 12, 2016

nguyenj commented Dec 12, 2016 • edited Loading

marcandre left a comment

Choose a reason for hiding this comment

marcandre Dec 12, 2016

Choose a reason for hiding this comment

nguyenj Dec 12, 2016

Choose a reason for hiding this comment

marcandre left a comment

Choose a reason for hiding this comment

marcandre Dec 12, 2016

Choose a reason for hiding this comment

marcandre Dec 12, 2016

Choose a reason for hiding this comment

nguyenj commented Dec 12, 2016 • edited Loading

marcandre commented Dec 12, 2016

marcandre commented Dec 12, 2016 via email

nguyenj commented Dec 12, 2016

nguyenj commented Dec 12, 2016

marcandre commented Dec 12, 2016 via email

marcandre commented Dec 22, 2016

nguyenj commented Dec 12, 2016 •

edited

Loading

nguyenj commented Dec 12, 2016 •

edited

Loading

nguyenj commented Dec 12, 2016 •

edited

Loading

nguyenj commented Dec 12, 2016 •

edited

Loading