Problem/Motivation

From #2962110-102: Add the Media Library module to Drupal core:

...the media tab lacks an empty table text the same as the other things under Content. We should add that for consistency.

This was replied to in #2962110-115: Add the Media Library module to Drupal core:

This table is provided by core media and is not altered by this patch. If we want to add empty table text we'd need to open a new issue and add an update hook since the View is already used on all sites using core media.

Let's look into that.

Proposed resolution

Provide empty text for the view, with an upgrade path.

Remaining tasks

Patch, test, review for usability, and commit.

User interface changes

Empty text will be added to a core view.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#7 interdiff-2981042-5-7.txt797 byteschr.fritsch
#7 2981042-7.patch3.33 KBchr.fritsch
#5 interdiff-2-5.txt808 bytesseanb
#5 2981042-5.patch3.27 KBseanb
#2 2981042-2.patch3.16 KBseanb

Comments

phenaproxima created an issue. See original summary.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Active » Needs review
StatusFileSize
new 2981042-2.patch3.16 KB

The default media view already had a empty text. Only the media library didn't have one. Patch attached adds a 'No media available.' text to the media library and changes the default text from 'No content available.' to 'No media available.' for the table view.

Not sure if we should add a update hook to change the text for existing sites. The library is not installed yet (if this makes it to 8.6) and changing the text for existing sites is probably not a good idea.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Update hooks are painful, especially when Views is involved. If it’s not essential, and I don’t think it is, let’s avoid it. :)

Status: Needs review » Needs work

The last submitted patch, 2: 2981042-2.patch, failed testing. View results

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Needs work » Needs review
StatusFileSize
new 2981042-5.patch3.27 KB
new interdiff-2-5.txt808 bytes

Another try.

Status: Needs review » Needs work

The last submitted patch, 5: 2981042-5.patch, failed testing. View results

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2981042-7.patch3.33 KB
new interdiff-2981042-5-7.txt797 bytes

Rerolled and fixed the tests

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

Sorry, I didn't want to change the status

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.
Even though the added asserts could be technically out of scope for this, we would need to find an empty scenario to test this anyway, and this increases our test coverage.
So +1 from me when it's green.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed c3615d2 and pushed to 8.6.x. Thanks!

  • alexpott committed c3615d2 on 8.6.x 
    Issue #2981042 by seanB, chr.fritsch, phenaproxima, marcoscano: Add...

Status: Fixed » Closed (fixed)

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