Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

Bug 1501282, add BCD survey #5133

Merged
merged 1 commit into from Nov 28, 2018
Merged

Bug 1501282, add BCD survey #5133

merged 1 commit into from Nov 28, 2018

Conversation

schalkneethling
Copy link

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 added the comp: frontend Component: Frontend label Nov 26, 2018
@Elchi3
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

Elchi3 commented Nov 26, 2018

https://bugzilla.mozilla.org/show_bug.cgi?id=1509833 it is

@schalkneethling
Copy link
Author

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

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected for me. 👍

});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this whitespace seems unnecessary

@schalkneethling
Copy link
Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
comp: frontend Component: Frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants