Fix/iframe attributes #269
Conversation
nguyenj
force-pushed
the
fix/iframe-attributes
branch
from
19e8e83 to
e12df42
Compare
|
Looks like good work 👍 I added a few comments. Also, please don't change |
src/featherlight.js
Outdated
| 'sandbox', 'src', 'srcdoc', 'width'], | ||
| attrs = {}; | ||
|
|
||
| whiteList.map(function(item) { |
There was a problem hiding this comment.
Why map? The return is unused...
src/featherlight.js
Outdated
| return result; | ||
| }; | ||
|
|
||
| function parseAttrs(obj, prefix) { |
There was a problem hiding this comment.
Please use correct indent
nguyenj
force-pushed
the
fix/iframe-attributes
branch
from
a11f783 to
da6e349
Compare
|
@marcandre thanks for the review. I've updated the code with your comments. |
nguyenj
force-pushed
the
fix/iframe-attributes
branch
from
da6e349 to
3e0e08a
Compare
|
Forgot, but there should be a test modified or added that checks this too... |
nguyenj
force-pushed
the
fix/iframe-attributes
branch
from
3e0e08a to
ef143e8
Compare
|
I added two tests, LMK. |
marcandre
left a comment
marcandre
left a comment
There was a problem hiding this comment.
Good first test. Second one needs work
test/featherlight_test.js
Outdated
| '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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I didn't add it to the CSS test because jQuery's .css should take care of invalid css attributes.
marcandre
left a comment
marcandre
left a comment
There was a problem hiding this comment.
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)
|
|
||
| if (attrValue) { | ||
| attrs[item] = attrValue; | ||
| return attrs[item]; |
|
|
||
| whiteList.map(function(item) { | ||
| $.each(whiteList, function(index, item) { | ||
| var attrValue = parseAttrs(obj, 'iframe')[item]; |
There was a problem hiding this comment.
Just noticed, but would be better to call parseAttrs once instead of once per item in whiteList...
nguyenj
force-pushed
the
fix/iframe-attributes
branch
from
64dc393 to
56e9ed8
Compare
|
I thought about doing that; but I wanted to leave the |
|
You could leave |
|
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>
.
|
|
Ah, okay I understand. I'll add those a test for that. |
|
I was thinking about refactoring |
nguyenj
force-pushed
the
fix/iframe-attributes
branch
from
56e9ed8 to
17e1253
Compare
|
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>
.
|
|
Merged, refactored a bit. Good work, thanks! Released in 1.7.0 |
2 participants
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.