Problem/Motivation

The sidebar in the Add or select Media Library widget is missing the proper styling in 10.2.x-dev and 11.x-dev:

add or select widget in 10.2 with no proper styling of the media typ sidebar

In 10.1.x the styling looks correct:

add or select widget in 10.1 with proper styling of the media typ sidebar

Steps to reproduce

1. Add a media field to a content type.
2. Add two or more media types as reference type for the added media field.
3. Create a node for that content type
4. Attempt to attach a media item to the field.
5. The Media Library "widget" View will display. The left sidebar, showing the available media types, does not display the previous horizontal separators.

Proposed resolution

Restore vertical tabs CSS custom properties to core/themes/claro/css/base/variables.pcss.css from core/themes/claro/css/components/vertical-tabs.pcss.css. Add comments documenting that the propertiers are used by both vertical tabs and media library. See #8

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 3394048.patch7.48 KBpraveenpb
#14 after-MR!6327.png1.65 MBdjsagar
#13 before-MR-!6327.png960.96 KBdjsagar
#13 after-MR-!6327.png881.71 KBdjsagar
#4 After.png78.42 KBsandeep sanwale
#4 3394048-3.patch3.4 KBsandeep sanwale
10.1.jpg88.3 KBrkoller
10.2.jpg66.98 KBrkoller

Issue fork drupal-3394048

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

    3 hidden branches
  • 3394048-drupal-10.2-regression Comparechanges, plain diff MR !6327
  • 10.2.x Comparecompare
  • 11.x Comparecompare
About issue forks

Comments

rkoller created an issue. See original summary.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
Title: The sidebar in the add or select media widget is missing a styling » [Drupal 10.2 regression] Media Library "widget" View media type tabs have lost styling
Issue summary: View changes

Sandeep Sanwale made their first commit to this issue’s fork.

sandeep sanwale’s picture

sandeep sanwale commented
Version: 11.x-dev » 10.2.x-dev
Status: Active » Needs review
StatusFileSize
new 3394048-3.patch3.4 KB
new After.png78.42 KB

In claro theme the base folder contains variable.pcss.css file which have root variables for other css files where variables for verticle tabs were missing which i have add it through this patch .

Sandeep Sanwale opened merge request !6327

daddison’s picture

daddison commented
Status: Needs review » Reviewed & tested by the community

The patch in #4 applies to 10.2.2 and restores the proper styling.

godotislate’s picture

godotislate
he/him
commented
Version: 10.2.x-dev » 11.x-dev
Component: media system » Claro theme
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs subsystem maintainer review, +media library, +Claro

The target branch should be set back to 11.x. Committers will backport to 10.2 if possible.

Also, some questions were raised in #3404866: Media Type links in Media Library modal missing vertical tabs styling in Claro which turned out to be a duplicate of this issue:
#50

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.

Possible paths forward:

  • Move the custom properties in Claro's vertical-tabs back to base variables, even though that means they will load on every request in the admin theme
  • Split out those custom properties to a separate css file so that the vertical tabs and media library libraries can share them. If so, where would the new css file live?
  • Decouple media library css from the vertical tabs custom properties?
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue tags: -Needs subsystem maintainer review

The bug was introduced trying to follow the pattern to moving the variables to its own component without knowing they would be needed by another one (Media Library). And with this changes we're getting back to the point were variables that belong to a component are on base.
The ideal solution would the one were the 2 components are really the same, but that's not doable. The second ideal solution would be to make these variables name generic enough to be used in two different components, but I'm afraid it might lead to some bikeshedding in the issue (naming tends to bring that).
So I'd focus on preventing it happens again apart from fixing the bug. I would recommend to to add a comment on the variables block that explains those variables are needed in 2 places, both Vertical Tabs and Media Library.

godotislate’s picture

godotislate
he/him
commented
Issue summary: View changes
Status: Needs work » Needs review

Added commit to MR with comment per #8 and removed properties from vertical tabs files.

Pinged in slack for a MR target change to 11.x

godotislate changed the visibility of the branch 10.2.x to hidden.

godotislate changed the visibility of the branch 11.x to hidden.

godotislate’s picture

godotislate
he/him
commented
djsagar’s picture

djsagar commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new after-MR-!6327.png881.71 KB
new before-MR-!6327.png960.96 KB

Hi all,

Steps to reproduce for the issue.
1. Drupal Version 11.x
2. Administration theme Claro
3. Add a media field to a content type.
4. Add two or more media types as reference type for the added media field.
5. Create a node for that content type
6. Attempt to attach a media item to the field.
7. Applied MR !6327

Result:
Before MR
before MR

After MR
After MR

After MR

RTBC ++

djsagar’s picture

djsagar commented
StatusFileSize
new after-MR!6327.png1.65 MB
praveenpb’s picture

praveenpb commented
StatusFileSize
new 3394048.patch7.48 KB
duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Issue tags: +Needs subsystem maintainer review

I can confirm that the proposed patch/MR fixes the issue.
I agree with @ckrina that this is not the ideal solution, though. I wonder if it wouldn't be better to create a new library for vertical-tabs styling and make both vertical tabs and media library libraries depend on it. Any thoughts, boss? :)

  • lauriii committed 89fde7a6 on 11.x 
    Issue #3394048 by godotislate, Sandeep Sanwale, djsagar, rkoller,...

  • lauriii committed 8581ca6f on 10.2.x 
    Issue #3394048 by godotislate, Sandeep Sanwale, djsagar, rkoller,...

lauriii closed merge request !6327

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs subsystem maintainer review

Looks like the bug is fixed and #8 has been addressed. Would be nice for sure to have a common component between these two use cases but it doesn't have to happen here.

Committed 89fde7a and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

idebr’s picture

idebr commented

This issue was released in Drupal 10.2.4, see https://www.drupal.org/project/drupal/releases/10.2.4