Issue 5538 - Implement "Help" tab for new options page

skin/new-options.css

Index: skin/new-options.css
===================================================================
--- a/skin/new-options.css
+++ b/skin/new-options.css
@@ -425,7 +425,6 @@
 button[role="checkbox"],
 #dialog-body .table button[role="checkbox"],
 .table button.delete,
-#content-help a::before,
 #dialog-close::before,
 #all-filter-lists-table .arrow,
 .context-menu .content a::before
@@ -466,7 +465,7 @@
   width: 12px;
 }
 
-#content-help a::before
+#dialog-body button::before
 {
   background-position: 0px -42px;
   content: "";
@@ -898,37 +897,53 @@
   Help tab content
 */
 
-#content-help
+html:not([lang="zh"]) #social-chinese,
+html[lang="zh"] #social-general
 {
-  counter-reset: section;
+  display: none;
 }
 
-#content-help h1::before
+#social ul

[ire, 2017/08/24 09:17:47]

Should there be the default padding-left on this element?

[saroyanm, 2017/08/24 13:22:28]

I think no, but this will be fixed with the sections separately, let's leave it
right now as it is.

[ire, 2017/08/25 08:52:49]

Acknowledged.

 {
-  counter-increment: section;
-  content: counter(section) ". ";
+  list-style: none;
 }
 
-#content-help a
+#social ul li

[ire, 2017/08/24 09:17:47]

The spacing between each list item is off/inexistent

[saroyanm, 2017/08/24 13:22:28]

Same here, I'm planing to touch this styles when styling the sections.
This styles may or may not share common styles with other elements, not yet
sure. So let's tackle that in the sections review.

[ire, 2017/08/25 08:52:50]

Acknowledged.

 {
-  color: #3A7BA6;
   display: inline-block;
-  text-decoration: none;
-  -moz-margin-end: 16px;
-  -webkit-margin-end: 16px;
-  vertical-align: top;
 }
 
-#content-help a::before
+#social ul li a

[ire, 2017/08/24 09:17:47]

The text isn't centered below the icon. Adding `text-align: center` here or on
the `li` will fix that.

[ire, 2017/08/24 09:17:48]

I'm not sure that the styling of these links will be covered by the general
style guide issue you mentioned (5543), because they appear to deviate. Should
the styling of these then be handled in this issue?

[saroyanm, 2017/08/24 13:22:28]

I don't have strong opinion, as this styles are missing in the style guide. I'll
stay away of addressing questionable style implementation in current review, as
this review should address Strings and HTML, styles will be revisited
separately, similar to other tabs. 

[saroyanm, 2017/08/24 13:22:28]

I'll update.

[ire, 2017/08/25 08:52:50]

Acknowledged.

 {
-  -moz-margin-end: 6px;
-  -webkit-margin-end: 6px;
+  display: block;
 }
 
-#share-general:lang(zh),
-#share-chinese:not(zh)
+#social ul li a::before
 {
-  display: none;
+  display: block;
+  margin: 0em auto;
+  width: 50px;

[ire, 2017/08/24 09:17:47]

Should this size be an em size instead? Feels like the size of the icons should
be related to the font size in this case.

[saroyanm, 2017/08/24 13:22:28]

Yes, it should I'll update this.

[saroyanm, 2017/08/24 18:19:42]

done.
Root item font-size = 20px;

[ire, 2017/08/25 08:52:50]

Acknowledged.

+  height: 50px;
+  content: "";
+  background-color: #099DD1;
+}
+
+#twitter::before
+{
+  -webkit-mask: url(social/twitter.svg);
+  mask: url(social/twitter.svg);

[ire, 2017/08/24 09:17:47]

Would using a plain background-image (or better yet an image in the markup) be
better for browser compatibility here? Although you would have to specify the
colour in the image itself and not CSS, it doesn't look like these images are
being used elsewhere so that shouldn't be an issue.

You know better than me on the browser requirements here so use your own
judgment.

[ire, 2017/08/24 09:17:47]

NIT: quotes (") around the url

[saroyanm, 2017/08/24 13:22:28]

Yes, I did it this way as the images are missing color and considering the fact
that the browser support is not an issue in this case, as we target Modern
Chrome and Firefox.

Specifing SVG in HTML will affect readability, having specified as a background
image, will require to produce more images for each color, for example if it
will be required to change the color on hover, we will need to import another
image if. So this solution seems to be good enough in my opinion for the launch
as it provides flexibility and do not affect readability.

[saroyanm, 2017/08/24 13:22:28]

Well spotted I'll change.

[saroyanm, 2017/08/24 18:19:42]

Done.

[ire, 2017/08/25 08:52:50]

Acknowledged.

+}
+
+#facebook::before
+{
+  -webkit-mask: url(social/facebook.svg);
+  mask: url(social/facebook.svg);
+}
+
+#google-plus::before
+{
+  -webkit-mask: url(social/googleplus.svg);
+  mask: url(social/googleplus.svg);
 }
 
 /*