Initial Claro design for the modules page

Postponing this until the design is done.

Could we credit @harishkunta and @sauravk on this? They began making improvements in #3116270: Fix CSS style in modules page, which I closed as a duplicate of this. They were not aware that the designs for the Modules weren't yet completed. I think it was a good faith effort in a process that probably isn't immediately clear.

I'm working on standardizing the design in the Figma to use the same details pattern as the Status Report.

Here is the updated design. This includes the updated component for Details and an update to the filter component for consistency with the Content page and better segmentation for clarity.

The designs look good visually. ✨ I'm wondering if we actually need the details element for the groups since they are open by default? I guess we don't have data on whether they are being used. If we think we need these variations of the details elements, we should probably also add those variations to the component specification https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system....

Re #12

I'm wondering if we actually need the details element for the groups since they are open by default? I guess we don't have data on whether they are being used.

When test modules are configured to be visible, their details container defaults to closed.

1. Could the Figma comp be updated to remove the secondary tabs for selecting a theme? I'm guessing this is just a copy/paste artifact
2. Since the nested details elements contain more than plain text, could the designs be expanded to cover not-just-plain-text contents such as list items and icon links such as the ones shown here for Configuration Manager?
   

When test modules are configured to be visible, their details container defaults to closed.

Thanks for pointing that out 👍 I guess we could still render the test modules using details. Having test modules visible is an edge case which we probably shouldn't try to accommodate too hard. I'm fine with all of the groups being rendered in details, but I would recommend reconsidering that if the only reason to do so is that we want to close the testing group.

Re #13

1. I'm guessing it was from a copy/paste as well. I've fixed it.
2. I've added an example for the icon links to the Figma.

Re #12

I remember that I found the ability to close the different categories useful when I was a Drupal site builder, but I don't have a strong argument for either direction.

1. I'm not seeing the changes in #16 in Figma using the link in the issue summary, perhaps theres a different location for this updated version? The Figmas are vast worlds and may have missed it.
2. I noticed the inline link styling in the top text (Download additional *contributed modules*, etc.) is different from inline link styling elsewhere.

   
   It looks like they more resemble .admin-item__link, which are for links not displayed inline, like the config items at admin/config.

   Wasn't sure if the deviation from the pattern was intentional, if there's an additional pattern I'm not aware of, or if the links should confirm with inline links elsewhere.

3. Looking at the Help/permissions/configure links, I'm not sure this styling for icon+link appears anywhere else in Claro, it seem like a mixture of styles for inline links and action links. admin/appearance uses the action links pattern so I'm wondering if that would work here as well (like the previous item, it's also possible I'm overlooking an existing pattern)

1. The updated component is here: https://www.figma.com/file/NpZUAp8BEkMJvm5gKBCDBB/Claro?node-id=17544%3A327

The Extend page updates should be visible now, they were in a different location.

2. That's an interesting point. The inline links in the Extend Figma match most of the pages in the Claro Figma, but none of those pages match the current Claro inline link styling. I'll update the Extend and Status Report pages. I'm guessing the Claro Figma just hasn't been updated since inline links were finalized.

3. I think that's a good idea.

I've updated the Details Component to use the Action Link components.

Really nice designs @DyanneNova!

I'm wondering if we actually need the details element for the groups since they are open by default?

I'd say they are necessary for smaller screens: it's too much info and would take too much vertical space otherwise, making it really difficult to navigate. Probably most people will use the search, but still it'd take ages to scroll.
And I'm not in favor of collapsing them by default on desktop, although it might be worth testing it on a design perspective.

Discussed the nested details elements with @ckrina and @saschaeggi. We agreed that the current solution is good enough for now, but we should open a follow-up to do a more invasive redesign. We could use something like Module Filter as an influence for the redesign. Removing the needs design tag and replacing with a needs followup tag to open a follow-up for the future design work.

I have two things that may require design input, but that does not need to block review.

1. At narrower widths, the "show all columns" link becomes visible. A design is needed for this link.
2. The module label color is #232429. This color not used anywhere else in Claro. The current patch uses this value, but there may be an existing color that can be used instead.

LTR/RTL full page screenshots attached.

Seems like the action links in the designs are using variation where the font-size is 12px. Is that a new variation of action-link because I cannot find that form the components?

The mockup is using the XS Text size, to match the description text. The Action Links components only has Medium and Small listed, but if we want to match the action links to the surrounding text size, we'll need an XS action link as well.

The module label color is the Text color in the Figma documents. It's still listed as the primary text color. Should we update the Figma to change the text color?

I've added a mockup for #22 - 1 to Figma. It includes an action link for Show all Columns.

The specification for this is now complete and can be taken into account in the implementation: https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system...

I've updated the patch with the design for the "show all columns" link, adjusted some spacing and color, and added the small variation of the action links in the module details element.

In the process I defined a variable for details-line-height because it's reused a few time to reset that value. I changed it to rem units instead of em. I don't *think* that creates a problem anywhere, but I'd love for someone else to confirm that in case I have missed something.

I'm a bit concerned about the implementation of the details element hover state below.

Highlighting the whole row across three table cells seems to introduce a lot of complexity that I worry may be a bit fragile and unsustainable. And since the module description details element is really a completely separate thing from the module name and checkbox, it seems like having them all hover together is an unexpected result. Focus, for example, doesn't work that way.

If feels like the natural behavior would be to either highlight the details element or the whole row, but it could be that I'm not thinking of the right solution, in which case I would love input.

For example:

The modules page will also ultimately benefit from #3057772: Ambiguous icons for collapsing/expanding menu items, so I didn't address that here.

I'm getting some errors with the latest patch:

User error: "module_package_listing" is an invalid render array key in Drupal\Core\Render\Element::children() (line 97 of core/lib/Drupal/Core/Render/Element.php).
Drupal\Core\Render\Element::children(Array) (Line: 708)
claro_form_system_modules_alter(Array, Object, 'system_modules') (Line: 449)
Drupal\Core\Theme\ThemeManager->alterForTheme(Object, 'form', Array, Object, 'system_modules') (Line: 458)
Drupal\Core\Theme\ThemeManager->alter(Array, Array, Object, 'system_modules') (Line: 837)
Drupal\Core\Form\FormBuilder->prepareForm('system_modules', Array, Object) (Line: 279)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 158)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 34)
Drupal\form_test\StackMiddleware\FormTestMiddleware->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 49)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 706)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
require('/Users/lauri.eskola/Projects/drupal/index.php') (Line: 65)

We could consider the focus effect from Gin because at least in my opinion it looks visually a bit nicer:

This should fix the previous patch test error, and I removed the comment noise.

The property change in #32 needed to have the corresponding conditionals that referenced it changed as well. Also changed the details arrow positioning so it's always in the upper corner instead of 50% of the element height (this becomes apparent on module descriptions long enough to wrap), and changed a stray px value to rem.

I think this is looking good. The mobile view looks like it's working correctly now and the details hover looks much better.

1. 
   Is it intentional that the width of the details element differs across groups? In Seven the groups have consistent width across all groups and I didn't any discussion about that here.
2. 
   The details elements resize sometimes when they are opened. I'm wondering if this could be avoided since it's not something that usually happens with the details elements.
3. 
   I feel like the design looks a bit busy in this state. There are various different margins, font-sizes and font-weights. Any thoughts if we tried to optimize some of those?
4. #21 still needs follow-up.

@#36

1. That's a good point. In Seven the first two columns have set % widths so that doesn't happen.

2. We should be able to avoid that by adding the fix for #1

3. I think the design is not any busier than Seven at this point and will get us to something that looks like Claro, so it's at least a good interim step. It sounds like we already want to update this page, so I think we can do more in the follow-up.

4. I've made a follow-up here #3175856: Redesign Extend Page in Claro

This should address #1 and #2 in #36.

#38 covers #1 and #2, so I think this is ready to go. The column widths match those of Seven now and the elements don't shift around.

Has the followup been made? It was asked for in #21.

Ah, I finally found it, it was made in #37. Removing tag.

I agree that this is much better than the Seven/Claro mix we had before. I made this comparison screenshot:

Attempted to commit this, got this error:

themes/claro/css/components/system-admin--modules.pcss.css
 56:3  ✖  Expected "color" to come before "border-bottom"   order/properties-order

Fixed #42 by running yarn run lint:css --fix. These changes are minor so moving back to RTBC.

Forgot to build CSS again 🤦‍♂️

Superb. Verified that the interdiffs were indeed accurate. Thanks all! As I wrote in #41 I agree this is a huge improvement.