fix(meta-viewport): show attribute in message #1061
Conversation
marcysutton
force-pushed
the
meta-viewport-msg
branch
from
c86df60 to
9538e81
Compare
| @@ -26,10 +26,12 @@ if ( | |||
| } | |||
|
|
|||
| if (!lowerBound && result['user-scalable'] === 'no') { | |||
| this.data(['user-scalable=no']); | |||
There was a problem hiding this comment.
Why is this an array? Is so should we do a join in the doT templates?
There was a problem hiding this comment.
I'd followed another example, there can only be one result but that looked like what doT was expecting.
| return false; | ||
| } | ||
|
|
||
| if (result['maximum-scale'] && parseFloat(result['maximum-scale']) < minimum) { | ||
| this.data(['maximum-scale']); |
There was a problem hiding this comment.
Are we saying if user-scalable=no is already set on data, maximum-scale will then override the data? Should we not concat?
There was a problem hiding this comment.
It returns false. There can only be one result.
lib/checks/mobile/meta-viewport.json
Outdated
| @@ -8,7 +8,7 @@ | |||
| "impact": "critical", | |||
| "messages": { | |||
| "pass": "<meta> tag does not disable zooming on mobile devices", | |||
| "fail": "<meta> tag disables zooming on mobile devices" | |||
| "fail": "{{~it.data:value}} {{=value}}{{~}} on <meta> tag disables zooming on mobile devices" | |||
There was a problem hiding this comment.
data seems to be an array, values should be joined here to construct the string...
There was a problem hiding this comment.
Changed it to a string.
There was a problem hiding this comment.
We need to do {{=it.data}} otherwise you get this:
| }); | ||
| describe('; separator', function() { | ||
| it('should return false on user-scalable=no', function() { | ||
| fixture.innerHTML = | ||
| '<meta name="viewport" content="foo=bar, cats=dogs, user-scalable=no">'; | ||
| var node = fixture.querySelector('meta'); | ||
|
|
||
| assert.isFalse(checks['meta-viewport'].evaluate(node)); | ||
| assert.isFalse(checks['meta-viewport'].evaluate.call(checkContext, node)); | ||
| assert.deepEqual(checkContext._data, ['user-scalable=no']); |
There was a problem hiding this comment.
can we add a test case where both user-scalable and maximum-scale are added to _data?
marcysutton
force-pushed
the
meta-viewport-msg
branch
from
3dabe65 to
f30fefa
Compare
marcysutton
force-pushed
the
meta-viewport-msg
branch
from
f30fefa to
713c481
Compare
marcysutton
force-pushed
the
meta-viewport-msg
branch
from
713c481 to
545dfaa
Compare
|
@WilcoFiers good catch, I've updated the PR and rebased from develop. |
|
@marcysutton seems like we should add some tests to ensure that the appropriate data is coming through? |
|
@dylanb not a bad idea, do you know of any existing places in the codebase where we test the messages? |
A user reported the meta-viewport check had a confusing failure message, so I changed it to pass along the failing attribute:
"Fix the following: user-scalable="no" on tag disables zooming"
Closes #1045
Reviewer checks
Required fields, to be filled out by PR reviewer(s)