Rewrite help.module component's inline with our CSS standards

Hi LewisNyman,

I think the more link should be viewed as a sub-component of help.

Should be changed to .help__more-link to avoid repeating the component context "help" unnecessarily.

Attached is patch with this suggestion.

Let me know what you think.

Thanks,

Ups,

Uploaded wrong patch in the previous comment.

Here goes the correct one.

Cheers,

Hi,

Here is a patch for what we discussed:

1. We use the generic more-link class instead of the help specific more-help-link that does the same thing.
2. To avoid targeting the anchor we use a generic icon-help class.For that I also had to add that in the forum.module
3. Since the only thing left in the help.css component is a font-size rule and an override of the paragraphs in the help section that doesn't seem to serve any purpose we also delete component itself and use the base paragraph margins and font-size.

I am attaching screens for before and after of the help screen (where the .help p and .help font-size and margin rules have an effect) we well as screens for the more link replaced with generic icon class and more-link class.

Hi,

Here is the updated patch with the reference to help.css removed from seven.libraries.yml

1. +++ b/core/modules/forum/forum.module
   @@ -57,12 +57,15 @@ function forum_help($route_name, RouteMatchInterface $route_match) {
   +        '#attributes' => array(
   +            'class' => array('icon-help')
   

   Drupal coding standards use two spaces for indentation instead of four. Also a multiline array assignment should end in a comma, eg. array('icon-help'), instead of array('icon-help')

2. +++ b/core/modules/forum/forum.module
   @@ -57,12 +57,15 @@ function forum_help($route_name, RouteMatchInterface $route_match) {
   +        )
   

   This line should end in a comma as well for coding standards.

3. +++ /dev/null
   @@ -1,15 +0,0 @@
   -.help {
   -  font-size: 0.923em;
   -}
   -.help p {
   -  margin: 0 0 10px;
   -}
   

   This removes the styling for every help text in Seven. I think this is out of scope for a 'Rewrite CSS to coding standards' type of issue?

Hi idebr,

Thanks for this review!

1. Will fix.
2. Will fix.
3. Why not use the base font-size and base paragraph here? The only thing this rule is doing is lowering the font-size by a marginal amount and overriding default margin for no apparent reason. I guess you maybe right in this being out of scope, so let me know if I leave it or if you want to open a separate issue for this.

@kyuubi Re #3: There is already a separate issue to remove small font sizes, so let's leave that part out here: #2045473: Improve visibility of Seven's smallest font elements

Hi idebr,

Ok will leave that one out and will put a patch with the rest later tonight.

Thanks!

Hi guys,

Here is the patch with that we discussed.

Also attached interdiff.

Removing the more-help-link and replacing it with icon-help needs a CR.

Hi Alex,

The help-more-link was replaced with the generic more-link

The 'help-more-link a' was replaced with 'icon-help'.

Do you mean the latter?

Thanks,

@LewisNyman we're completely removing a class - this needs a CR as some module author might be relying on the this functionality. And I've been asking for CRs on issues that are removing classes before.

@alexpott Added the CR:

https://www.drupal.org/node/2462579

Let me know if everything is in order.

+++ b/core/modules/forum/forum.module
@@ -57,12 +57,15 @@ function forum_help($route_name, RouteMatchInterface $route_match) {
-          'class' => array('more-help-link'),
+          'class' => array('more-link'),

The CR needs to mention that more-link replaces more-help-link. And that the icon-help is used on text inside the container.

It totally makes sense to have CRs for removed core classes, and it would be great to have them indexed in one CR to allow contrib themes to follow core changes. Especially for themes that used for administration

Hi,

I updated the CR to reflect the changes to the more-link as well.

Also change title to be more generic in order to incorporate other eventual changes to this component.

Upload the reroll.

I'm not very sure about put 'reroll' in the syntax of the patch. But, wait your answer.
Thank you!

Here is the new patch!

This is a CSS only change permitted in beta. Committed 90e7bdd and pushed to 8.0.x. Thanks!