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 1501282, add BCD survey #5133

Merged
merged 1 commit into from Nov 28, 2018

Conversation

Projects
None yet
3 participants
@schalkneethling
Copy link
Collaborator

schalkneethling commented Nov 26, 2018

This adds the new survey to the BCD table. This is ready for code review. Still need to confirm the following:

  1. Copy for the survey link
  2. Whether we want to open the survey in a new tab, or the current tab

@Elchi3 r?

@schalkneethling schalkneethling requested a review from Elchi3 Nov 26, 2018

@Elchi3

This comment has been minimized.

Copy link
Contributor

Elchi3 commented Nov 26, 2018

Thanks Schalk! 👍

As I said in the issue, I would go with "Take the quick survey" (but maybe @atopal or @shilili have another optimized wording). Otherwise, I would go with whatever we usually put in the soapbox. Same for new tab or not. If I remember well, the soapbox had links opened in a new tab.

I managed to test this locally and it works fine. When there are multiple compat tables on a page, the survey is only shown for the first table, which I think is good.

webext-compat

This screenshot is from https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Browser_support_for_JavaScript_APIs. I'm not sure if we actually tested labels and icons for this page, it looks a bit off, because there are a lot less browsers in WebExtension tables. We might want to do something about this.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 26, 2018

This screenshot is from developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Browser_support_for_JavaScript_APIs. I'm not sure if we actually tested labels and icons for this page, it looks a bit off, because there are a lot less browsers in WebExtension tables. We might want to do something about this.

Oh wow, I have actually not seen this before :/ I reckon it makes sense to open an issue to look into better spacing for the text labels.

@Elchi3

This comment has been minimized.

Copy link
Contributor

Elchi3 commented Nov 26, 2018

Do you want a new Bugzilla bug for this? Or an mdn/sprint issue? Or should I comment this issue on https://bugzilla.mozilla.org/show_bug.cgi?id=1438889 ? Or something else? :) (yay processes 😄)

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 26, 2018

Do you want a new Bugzilla bug for this?

Kuma still requires a Bugzilla bug so, a new Bugzilla bug assigned to me would be great. If you could also highlight other pages that deviates from the norm, that would be extra bonus points ;p Thanks!

@Elchi3

This comment has been minimized.

Copy link
Contributor

Elchi3 commented Nov 26, 2018

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1501282-add-bcd-survey branch from 029ec76 to a733307 Nov 26, 2018

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 26, 2018

@Elchi3 @hobinjk ok, this has been updated with the final of all the things. Ready for review. Thanks!

@Elchi3

Elchi3 approved these changes Nov 26, 2018

Copy link
Contributor

Elchi3 left a comment

Works as expected for me. 👍

@jwhitlock jwhitlock requested a review from hobinjk Nov 26, 2018

});

This comment has been minimized.

@hobinjk

hobinjk Nov 27, 2018

Collaborator

nit: this whitespace seems unnecessary

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1501282-add-bcd-survey branch from a733307 to 16933d3 Nov 28, 2018

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 28, 2018

Thanks for the reviews @Elchi3 and @hobinjk - waiting for Travis to be green and then merging.

@schalkneethling schalkneethling merged commit b2ed07c into mozilla:master Nov 28, 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

@schalkneethling schalkneethling referenced this pull request Nov 29, 2018

Closed

Fix browser labels for WebExtension compat tables #680

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment