New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1438889, switch to text labels and icons for BCD table headers #5117

Merged
merged 2 commits into from Nov 21, 2018

Conversation

Projects
None yet
4 participants
@schalkneethling
Copy link
Collaborator

schalkneethling commented Nov 19, 2018

This is the Kuma side of KumaScript mdn/kumascript#997 It changes the browser icons to text labels with accompanying icons. Also switched to all SVG, and thus removes custom web font.

Before

screenshot 2018-11-19 at 17 01 47

After

screenshot 2018-11-19 at 17 02 12

@jwhitlock @hobinjk r?

@hobinjk
Copy link
Collaborator

hobinjk left a comment

Changes LGTM although I think I'm unable to test without the corresponding kumascript updates

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 19, 2018

Thanks @hobinjk - Forgot to update the description with the link to the KumaScript PR. Here it is mdn/kumascript#997

@hobinjk

This comment has been minimized.

Copy link
Collaborator

hobinjk commented Nov 20, 2018

Trying to run this locally with the kumascript PR checked out I'm getting
screen shot 2018-11-20 at 10 44 01 am
with this HTML source:

<table class="bc-table bc-table-js">
    <thead>
        <tr class="bc-platforms">
            <td></td>
            <th colspan="6" class="bc-platform-desktop"><span>Desktop</span></th>
            <th colspan="7" class="bc-platform-mobile"><span>Mobile</span></th>
            <th colspan="1" class="bc-platform-server"><span>Server</span></th>
        </tr>
        <tr class="bc-browsers">
            <td></td>
            <th class="bc-browser-chrome"><span class="bc-head-txt-label bc-head-icon-chrome">Chrome</span></th>
            <th class="bc-browser-edge"><span class="bc-head-txt-label bc-head-icon-edge">Edge</span></th>
            <th class="bc-browser-firefox"><span class="bc-head-txt-label bc-head-icon-firefox">Firefox</span></th>
            <th class="bc-browser-ie"><span class="bc-head-txt-label bc-head-icon-ie">Internet Explorer</span></th>
            <th class="bc-browser-opera"><span class="bc-head-txt-label bc-head-icon-opera">Opera</span></th>
            <th class="bc-browser-safari"><span class="bc-head-txt-label bc-head-icon-safari">Safari</span></th>
            <th class="bc-browser-webview_android"><span class="bc-head-txt-label bc-head-icon-webview_android">Android webview</span></th>
            <th class="bc-browser-chrome_android"><span class="bc-head-txt-label bc-head-icon-chrome_android">Chrome for Android</span></th>
            <th class="bc-browser-edge_mobile"><span class="bc-head-txt-label bc-head-icon-edge_mobile">Edge Mobile</span></th>
            <th class="bc-browser-firefox_android"><span class="bc-head-txt-label bc-head-icon-firefox_android">Firefox for Android</span></th>
            <th class="bc-browser-opera_android"><span class="bc-head-txt-label bc-head-icon-opera_android">Opera for Android</span></th>
            <th class="bc-browser-safari_ios"><span class="bc-head-txt-label bc-head-icon-safari_ios">iOS Safari</span></th>
            <th class="bc-browser-samsunginternet_android"><span class="bc-head-txt-label bc-head-icon-samsunginternet_android">Samsung Internet</span></th>
            <th class="bc-browser-nodejs"><span class="bc-head-txt-label bc-head-icon-nodejs">Node.js</span></th>
        </tr>
    </thead>
    <tbody>
        <tr>
            <th scope="row">Basic support</th>
            <td class="bc-supports-yes bc-browser-chrome"><span class="bc-browser-name">Chrome</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-edge"><span class="bc-browser-name">Edge</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-firefox"><span class="bc-browser-name">Firefox</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> 1.5
            </td>
            <td class="bc-supports-yes bc-browser-ie"><span class="bc-browser-name">IE</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> 9
            </td>
            <td class="bc-supports-yes bc-browser-opera"><span class="bc-browser-name">Opera</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-safari"><span class="bc-browser-name">Safari</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-webview_android"><span class="bc-browser-name">WebView Android</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-chrome_android"><span class="bc-browser-name">Chrome Android</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-edge_mobile"><span class="bc-browser-name">Edge Mobile</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-firefox_android"><span class="bc-browser-name">Firefox Android</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> 4
            </td>
            <td class="bc-supports-yes bc-browser-opera_android"><span class="bc-browser-name">Opera Android</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-safari_ios"><span class="bc-browser-name">Safari iOS</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-samsunginternet_android"><span class="bc-browser-name">Samsung Internet Android</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
            <td class="bc-supports-yes bc-browser-nodejs"><span class="bc-browser-name">nodejs</span><abbr class="bc-level-yes only-icon" title="Full support">
                <span>Full support</span>
              </abbr> Yes
            </td>
        </tr>
    </tbody>
</table>

I rebuilt all the static files and force-refreshed the page a few times so I'm not sure what I'm missing

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 20, 2018

@hobinjk Hmmm, that is strange. The HTML looks good but the CSS is definitely not there. Do you have both the Kuma and KumaScript pull requests checked out?

@hobinjk

This comment has been minimized.

Copy link
Collaborator

hobinjk commented Nov 20, 2018

@schalkneethling My bad, I just forgot to restart the web task after building everything.

Now that I have it going it looks good on wide screens and mobile-scale screens. The one issue I'm seeing is on some screen widths (between 770px-1500px wide) the table gets squished and pushes the icons/text off center:
screen shot 2018-11-20 at 11 22 59 am

@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 20, 2018

My first impression is that all the icons are rotated 90°, but we can ship it, and I'll get used to it.

@schalkneethling I've approved mdn/kumascript#997, and it looks like it should go the same time as this PR. Let me know if you want to merge as it is, or look into the tablet width style.

@Elchi3

This comment has been minimized.

Copy link
Contributor

Elchi3 commented Nov 20, 2018

My first impression is that all the icons are rotated 90°

I also think icons shouldn't be rotated. It makes this harder to read imo.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 21, 2018

Thanks for the reviews all, I will address these issues and update the PR.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 21, 2018

Updated, thanks!

screenshot 2018-11-21 at 10 30 32

@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 21, 2018

The screenshot looks good. @hobinjk, will you have time to re-test your 770px-1500px wide issue?

@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 21, 2018

OK, we'll address any tablet style issues in the next PR.

@jwhitlock jwhitlock merged commit ff349d9 into mozilla:master Nov 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No new, high severity issues
Details
@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 21, 2018

This PR is on staging:

https://developer.allizom.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#Browser_compatibility

However, the headings disappear until the page is regenerated.

screenshot 2018-11-21 15 14 12

We're waiting until after the US Thanksgiving holiday to push to production. If the BCD survey requires regenerating the page as well, maybe both can go out at the same time.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 22, 2018

If the BCD survey requires regenerating the page as well, maybe both can go out at the same time.

The survey bit won't require pages to be regenerated as it will be dynamically done via JS. Thanks @jwhitlock

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment