i18n: tap targets #7111
i18n: tap targets #7111
Conversation
Global one tries to match any content following `module.exports.UIStrings = UIStrings;` and then backtracks, so if there's more content the process will hang.
mattzeunert
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
| @@ -13,7 +13,7 @@ const path = require('path'); | |||
| const esprima = require('esprima'); | |||
|
|
|||
| const LH_ROOT = path.join(__dirname, '../../../'); | |||
| const UISTRINGS_REGEX = /UIStrings = (.|\s)*?\};\n/gim; | |||
There was a problem hiding this comment.
If this regex is global it tries to match any content following module.exports.UIStrings = UIStrings; and then backtracks. If there's some content after that line the collect-strings script hangs.
There was a problem hiding this comment.
btw this no longer collects all matches, but that's fine cuz we only ever use the first instance.
| // eslint-disable-next-line | ||
| explanationViewportMetaNotOptimized: 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens', | ||
| /** Explanatory message stating that a certain percentage of the tap targets (like buttons and links) on the page are of an appropriately large size. */ | ||
| displayValue: '{decimalProportion, number, percent} appropriately sized tap targets', |
There was a problem hiding this comment.
is percent it's own thing already or do we need to make a custom one? I definitely see extendedPercent
There was a problem hiding this comment.
percent is a default iirc, it just prunes decimals which is why we needed extendedPercent. As long as we are okay with no decimals then we should be good with percent here. Might need to add this to the LR tc bundler though, since I don't think we've ever used just percent so far.
There was a problem hiding this comment.
Might need to add this to the LR tc bundler though, since I don't think we've ever used just percent so far.
What does this mean, and is it something I have to do?
There was a problem hiding this comment.
Anything left for me to do here? Or is this PR good to go?
There was a problem hiding this comment.
AFAICT, we never actually flex this in the tests, can we add an assertion for it looking like we expect? then I'm good :)
off to @exterkamp on the "LR tc" bit since I'm not 100% where that change would be made either
| /** Label of a table column that identifies a tap target (like a link or button) that overlaps with another tap target. */ | ||
| overlappingTargetHeader: 'Overlapping Target', | ||
| /** Explanatory message stating that there was a failure in an audit caused by the viewport meta tag not being optimized for mobile screens, which caused tap targets like buttons and links to be too small to tap on. */ | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
just line length? let's add max-len then to narrow it a bit
| // eslint-disable-next-line | ||
| explanationViewportMetaNotOptimized: 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens', | ||
| /** Explanatory message stating that a certain percentage of the tap targets (like buttons and links) on the page are of an appropriately large size. */ | ||
| displayValue: '{decimalProportion, number, percent} appropriately sized tap targets', |
There was a problem hiding this comment.
percent is a default iirc, it just prunes decimals which is why we needed extendedPercent. As long as we are okay with no decimals then we should be good with percent here. Might need to add this to the LR tc bundler though, since I don't think we've ever used just percent so far.
|
Need to update the test to use |
i18n for tap targets audit.