-
Notifications
You must be signed in to change notification settings - Fork 172
Fix Bug 1480106, on mobile, show browser name istead of icon #776
Fix Bug 1480106, on mobile, show browser name istead of icon #776
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit that needs fixing (r+wc) and I left two general comments that are just fyi.
I've tested in Firefox and Chrome Desktop with large and small resolutions and it works as intended. Good work!
macros/CompatBeta.ejs
Outdated
case 'ie': | ||
return browserNameKey.toUpperCase(); | ||
case 'webview_android': | ||
return 'Webview Android'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Webview -> WebView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
* @param {String} browserNameKey - The browser name | ||
* @returns Correctly formatted browser name | ||
*/ | ||
function getFormattedBrowserName(browserNameKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is OK for now. In the future, the formatted name should come from the compat data package.
(the export is browsers.browserNameKey.name
), but we're missing two browsers still (see mdn/browser-compat-data#2690 and mdn/browser-compat-data#1712)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that will make this much simpler.
@@ -496,7 +533,7 @@ function writeCompatCells(supportData) { | |||
output += ' bc-has-history'; | |||
} | |||
|
|||
output += `">${supportInfo}`; | |||
output += `"><span class="bc-browser-name">${browserName}</span>${supportInfo}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this hidden for now works for me. Note that we want to remove icons and use text everywhere and not just on mobile in the future. See #616. I'm not sure when we want to do this, though. Maybe now isn't the time for it. So this is OK with me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The scope of this was mobile specific, as that is where we will mostly see the use case of people turning of custom fonts.
Looks great now! Not merging, as I guess it requires merging with the Kuma PR at the same time. |
This fixes the problem on mobile viewports. sprints issue #390 - mdn/sprints#390
@Elchi3 r?