Media Type links in Media Library modal missing vertical tabs styling in Claro

Hi @ Nitin shrivastava, I have checked this issue on my end the issue is replicated in my local. Also applied MR 5583 and it is perfectly applied and working fine. The media modal is themed now but according to me, this issue should be fixed in the Claro theme as Claro is a default admin theme.

The issue also exists in the claro theme as well.

Below are some screenshots:-

Oliver before the patch -

Olivero after the patch -

Claro admin theme -

The merge request failed its pipeline.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Closed #3406681: Drupal Modal media library menu items should be designed in a better way as a duplicate.

If this is failing both themes a solution should be made to make the styling generic

Also issue summary isn't following standard template when it should

There were some variables missing in core/themes/claro/css/theme/media-library.pcss.css file. I have added them in the MR.

The MR fixed the issue for Claro theme.

So my comment was to see if a generic solution could be found, is this just fixing claro? What about olivero?

Should be documented in issue summary.

For Olivero, this could be a feature request rather than a bug. The Media Library dialog was never themed in Olivero. Also, could someone provide the steps to reproduce this issue without using any contrib module?

Here, I have attached a screenshot from an earlier version (10.x), and it is broken on that version as well.

Note: I am using Olivero as the admin theme to reproduce the bug. However, we do not support using Olivero as the admin theme.

The MR changes code only in Claro, so updating this to the Claro issue queue.

Agree with #18 comment as we do not support the Olivero theme as an admin theme. Updating status to NR
Thanks

IS still needs updating.

Issue summary updated also added standard template.
Thanks

Verified and tested patch MR !5583 mergeable on Drupal version-
11.0-dev. The patch was applied successfully and looks good to me.

Testing Steps:

1. Set up the Drupal 11 site.
2. I enabled the Claro theme for admin.
3. Enable the media module and Add a media field.
4. Click on add content type and insert media.
5. Apply the shared patch & reverify the results.

Testing Results:
Now the media Modal broken preview issue is fixed for the Claro theme, Although the issue is still there for the Olivero theme. We can move this to RTBC as this ticket is for the Claro theme.

Instead of introducing duplicates of the CSS variable defined in vertical-tabs.css we should aim to actually include the correct library (vertical tabs).

Moving this back to needs work.

@saschaeggi, I've included the vertical-tabs library in media-form. Please review.

@omkar-pd thank you for working on this. I've left one suggestion to prevent loading the library generally. Otherwise this approach looks good to me 👍

@saschaeggi @omkar-pd

This library $form['#attached']['library'][] = 'claro/vertical-tabs'; will only get added on the Media Library Form as it is already inside the

hook_form_BASE_FORM_ID_alter()

claro_form_media_library_add_form_alter(array &$form, FormStateInterface $form_state)

Therefore, in my opinion we do not need any additional check here.

Thanks

@Prashant.c

Therefore, in my opinion we do not need any additional check here.

We do as per default we don't have tabs in this dialog.

Yes, that's correct. Including the library fixes the issue but these are not vertical tabs.

We need help from some front-end dev here as adding the vertical-tabs library fixing the issue but the links/tabs within the media modal window are not vertical-tabs so IMO it is not a good idea to add this library here.

Thanks

Doing some archaeology:

In #3023767: Reuse or build on vertical tabs styling in media library, there was some thought of consolidating the media library menu and vertical tabs, whether through a shared CSS file, or refactoring the media library to actually use vertical tabs, but the conclusion was just to address media library menu CSS directly in the admin theme (then Seven).

In #3062751: Media and media library, the media library was re-designed in Claro. The vertical tabs CSS custom properties were from core/themes/claro/css/components/vertical-tabs.pcss.css to core/themes/claro/css/base/variables.pcss.css in this commit on 9.2.x.

In #3332729: Refactor Claro's vertical-tabs stylesheet, in the effort to refactor the vertical tabs CSS and putting component custom properties in the corresponding component CSS file, the vertical tabs custom properties were moved back from core/themes/claro/css/base/variables.pcss.css to core/themes/claro/css/components/vertical-tabs.pcss.css, without awareness that media library also depended on them.

I think it's best to move these variables back to core/themes/claro/css/base/variables.pcss.css and document why they need to live there. Pushed commit with this to the MR and rebased.

Screenshot before:

Screenshot after:

Verified and tested patch MR !5583 mergeable on Drupal version-
11.0-dev.
The patch was applied successfully and looks good to me.
Testing Steps:

1. Set up the Drupal 11 site.
2. I enabled the Claro theme for admin.
3. Enable the media module and media library and Add a media field.
4. Click on add content type and insert media.
5. Apply the shared patch & reverify the results.

Testing Results:
Warning/errors were not found after applying the patch and it is as per the coding standard.
Can be move to RTBC

Verified and tested MR !5583 on Drupal 11 and it is working fine. Media library tabs/links now look good. Moved Claro vertical-tabs CSS custom properties back to base variables.css

Attached after screenshot -

Moving to RTBC

Thanks

Hi, I tested MR !5583 on Drupal version 11.0
The patch was applied successfully
Testing Steps:

1. Set up the Drupal 11 site.
2. Set the Claro theme as a default theme
3. Enable the media and media library modules
4. Add a media field for the content type
5. Click on add content type and insert media.
6. Apply the shared patch & reverify the results.

The patch was applied successfully, and the media Modal broken preview issue is fixed for the Claro theme.
Can move to RTBC

Patch for 10.2.x for a temporary fix until the new version.

Facing the same issue.
Thank you for the MR and the local patch

Tested MR #5583 on Drupal 10.2.1 looks fine, thanx

Before this lands as is, worth checking solution in https://www.drupal.org/project/drupal/issues/3416545 as well.

@Graber - the media type links look like vertical tabs, but aren't actually vertical tabs. Adding the core/drupal.vertical-tab library fixes the CSS but adds unnecessary JS. See history of how the CSS got lost: #35

Ahh, makes sense. We should add this css conditionally and not globally in any case to optimize. Additional library with css only maybe?

I'm quite sure a subsystem maintainer will request the same - no point loading those variables on every page, those are component variables, be it vertical tabs or media library media type links. Those variables should be included conditionally, only if one or both components are present in the current context.

Looks like this is a duplicate of #3394048: [Drupal 10.2 regression] Media Library "widget" View media type tabs have lost styling.