Problem/Motivation

we have to many css floating around that dont do much more than add a bit of css to seven
Its confusing for everybody why & where it comes from so lets clean it up

search.admin.css dont contains anything that makes it critacal for it to function, its pure layout 

.search-admin-settings .container-inline {
  margin-bottom: 1em;
}
.search-admin-settings label[for="edit-search-type"] {
  display: block;
}

Proposed resolution

move the css over to seven, where it belongs

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#22 after_2.png87.93 KBChernous_dn
#22 before_2.png87.91 KBChernous_dn
#20 move_search_admin_css-2491259-20.patch2.5 KBChernous_dn
#14 move_search_admin_css-2491259-14.patch2.44 KBChernous_dn
#14 style.png125.81 KBChernous_dn
#8 before.png87.91 KBChernous_dn
#8 after.png88.11 KBChernous_dn
#7 move_search_admin_css-2491259-7.patch1.25 KBChernous_dn
#2 search-admin-css-2491259-2.patch955 bytesrudraram
#11 move_search_admin_css-2491259-11.patch2 KBChernous_dn
#11 error_stark.png6.53 KBChernous_dn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented

@mortendk the title of this issue should be move search.admin.css into seven.

You have already added the Issue for file.admin.css here #2491253: move file.admin.css into seven

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented
FileSize
search-admin-css-2491259-2.patch955 bytes

Added patch as per the proposed resolution

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented
Status: Active » Needs review
mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented
Title: move file.admin.css into seven » move search.admin.css into seven
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Looks probably OK, maybe some Before and After screen shots in Seven and Stark would be useful though?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work
Issue tags: +Needs screenshots
Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
Status: Needs work » Needs review
FileSize
move_search_admin_css-2491259-7.patch1.25 KB

Update patch #2 add search.admin.css to seven.libraries.yml

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
FileSize
before.png87.91 KB
after.png88.11 KB

Add screenshot.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks for the patch and screenshots! Can we also get screenshots in the Stark theme (before and after)?

Hopefully Cottser can review the patch as far as whether putting it in that place in Seven is the right thing to do. I'm not sure how Seven is organized.

But I think the patch also needs to change module/search/search.libraries.yml to take out the admin CSS file (and its library) there? And then I guess somewhere the library that is defined in there is referenced in some search.module code, so that reference would need to be removed as well?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

It looks like the reference to the attached library is in /core/modules/search/src/SearchPageListBuilder.php:

     '#attached' => [
        'library' => [
          'search/admin',
        ],
      ],

So that needs to be removed. And it seems like Seven should only be using this search.admin.css if you're on that page?

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
FileSize
error_stark.png6.53 KB
move_search_admin_css-2491259-11.patch2 KB

@jhodgdon thanks for the help. I update patch. But I can't check in the Stark theme. I have error (screenshot).

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: search.module » Seven theme
Status: Needs review » Needs work

@Chernous_dn: That Stark problem seems like a pretty serious bug! If it persists we should definitely file an issue.

Regarding the latest patch, I think we still need to remove the entry in the search.libraries.yml file that references this CSS file, right?

And I also would have thought that this CSS file in Seven would only want to be included when we're actually on the Search settings page... it seems like the way this patch is written, this CSS file would be included on every page that uses the Seven theme. I do not know if this is standard Seven behavior or not, but previously due to that #attached line in the Search class (referenced in previous comment), this CSS file was only being included if we were on that one page. ??? Possibly the Seven theme maintainers can comment here; since I (one of the Search maintainers) am already subscribed to this issue, I think I'll move this issue component to make sure the Seven maintainers see it.

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
FileSize
move_search_admin_css-2491259-14.patch2.44 KB
style.png125.81 KB

@jhodgdon thanks again. I update patch and remove in the search.libraries.yml references. I think it's normal CSS file included on every page, because the rest of the files are also on every page. (screenshot)

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks! Looks OK to me then. Still would like to see (a) Stark before/after screen shots and (b) comment from Seven maintainers about whether this CSS should be put where this patch puts it and used on every admin page.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Actually I'd like to see a screen shot from Bartik too.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work
Issue tags: -Needs screenshots

If this is admin only styling then we shouldn't test Bartik. We'd be taking on a heavy burden if we decided that Bartik supports all admin interfaces.

+++ b/core/themes/seven/seven.libraries.yml
@@ -25,6 +25,7 @@ global-styling:
+      css/components/search.admin.css: {}

The name of this file should match the class styled within it. In this case it's search-admin-settings.

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
Status: Needs work » Needs review
FileSize
move_search_admin_css-2491259-20.patch2.5 KB

@LewisNyman thanks. Update patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue tags: +Needs screenshots

Thanks! Given that this is a new patch, maybe we should have screen shots for Seven and Stark again please?

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
FileSize
before_2.png87.91 KB
after_2.png87.93 KB

@jhodgdon Add screen for Seven. For Stark this style not work.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

That's correct, Stark should not have any styling. I feel like this patch has been sufficiently tested. Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

What I was wondering was whether without the CSS that was previously in Stark because it was in search.module, the page is entirely broken in Stark. But perhaps we don't care about that... I guess that is the point of Stark.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

hmm, well the page shouldn't be broken, it should still be functional. It can look bad, but only as bad as unstyled HTML looks: http://info.cern.ch/hypertext/WWW/TheProject.html

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I just tested this patch in Stark. I would put it in the "ugly but usable" category. So let's go ahead, since Lewis has confirmed the CSS file has been moved to the right place now. Thanks!

alexpott’s picture

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

CSS is not frozen in beta. Committed 9ae518c and pushed to 8.0.x. Thanks!

  • alexpott committed 9ae518c on 8.0.x 
    Issue #2491259 by Chernous_dn, rudraram, jhodgdon, LewisNyman, mortendk...

Status: Fixed » Closed (fixed)

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