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

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

/* File upload widget.*/
.form-managed-file .form-submit {
  margin: 0 0.5em;
}
.form-managed-file div.ajax-progress-bar {
  display: none;
  margin-top: 4px;
  padding: 0;
  width: 28em;
}
.form-managed-file .ajax-progress-bar .bar {
  margin: 0;
}

Proposed resolution

move the css over to seven, where it belongs

Remaining tasks

User interface changes

API changes

Comments

rudraram’s picture

Status: Active » Needs review
StatusFileSize
new534 bytes

Moved css into seven as proposed above.

rudraram’s picture

StatusFileSize
new1.07 KB

Apologies. My patch above didn't capture all the changes. Updated the above patch.

chernous_dn’s picture

Status: Needs review » Needs work
Issue tags: +need screenshots
chernous_dn’s picture

Status: Needs work » Needs review
StatusFileSize
new1.73 KB
new4.92 KB
new3.05 KB
new4.83 KB
new3.1 KB
new112.17 KB

Update patch #2. Rename of file file.admin.css to form-managed-file.css. Match the class styled within. Add form-managed-file.css to seven.libraries.yml. Removed reference in file file.libraries.yml . Add screenshots.

mortendk’s picture

#5 Renaming css files it out of scope with this issue, lets not make things more complicated. We must first the css files over to the theme (classy or in this case seven) then take the discussion of what we want to name them.

chernous_dn’s picture

HI @mortendk in issue https://www.drupal.org/node/2491259 @LewisNyman recommended renaming file:

The name of this file should match the class styled within it.

I decided to do it.

mortendk’s picture

Issue tags: -need screenshots
StatusFileSize
new1.1 KB
new530 bytes

the problem by renaming files as "we feel like it" with no cencensus, is were gonna end up with the mess.
If we rename files as we go lets keep them as simple & desciptive as possible in this case form-file.css
Seven is naming files to be on a component level, afaik its not gonna be named after a class (that somebody came up with 8 years ago) .form-managed-file

What we agreed upon in LA at the meta discussion was to move all css files that we could move - then after that do the work of naming & placing the css files the right places.

Anyways lets not bikeshed this keep it simple as possible and get the stuff moved in, cause were running out of time...

lewisnyman’s picture

Status: Needs review » Needs work

@mortendk All the CSS files in Seven are named after the components within them, the classes are the component names. If you want to put this in Seven then you need to follow the conventions in the theme. Anyway you've already renamed the file from file.admin.css so what's the harm? We can bikeshed the real class name in another issue.

mortendk’s picture

upsie ;) lemme just reroll this *coughs* nothing to see here

mortendk’s picture

Status: Needs work » Needs review
StatusFileSize
new1.73 KB

reuploading the patch from #4
*whisteling*

lewisnyman’s picture

StatusFileSize
new1.73 KB

Reuploading as .patch

lewisnyman’s picture

Issue tags: +Needs screenshots

This is hard to test because a lot of this CSS affects the ajax-progress-bar class and I can't get my local server set up to display the filter upload progress.

Is anyone able to do this and take screenshots before/after?

chernous_dn’s picture

@LewisNyman I add screenshots in #4 comment.

lewisnyman’s picture

@Chernous_dn Is it possible for you to get of the upload progress bar?

chernous_dn’s picture

StatusFileSize
new5.51 KB
new3.38 KB
new5.76 KB
new3.54 KB
new4.66 KB

@LewisNyman Yes sure, add screenshots. I tested in file filed, the same result.

lewisnyman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

Top work! Thank you

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think that this needs to go Bartik too as the node form which can contain file fields can appear in bartik and same with the account forms since peeps can add file field to that too.

lewisnyman’s picture

Title: move file.admin.css into seven » move file.admin.css into Classy
Issue tags: +Classy, +Bartik

In that case, we move it to Classy

pjbaert’s picture

Status: Needs work » Needs review
StatusFileSize
new2.1 KB
new1.88 KB

I moved the File.admin.css to the classy css/components folder & as I decided to use the same analogy @LewisNyman mentioned in https://www.drupal.org/node/2489566#comment-10216757

chernous_dn’s picture

Status: Needs review » Needs work

@pjbaert I already create patch for Classy, we need create patch for Bartik.

chernous_dn’s picture

Status: Needs work » Needs review
StatusFileSize
new2.74 KB

Update patch, add style to Bartik theme.

lewisnyman’s picture

I'm confused, the patch in #19 is adding the file to Classy, and the patch in #21 is adding the same file to Bartik and Seven.

@Chernous_dn Is this what you intended? If we add the file to Classy then it will already be loaded in Bartik and Seven.

Status: Needs review » Needs work

The last submitted patch, 21: move_file_admin_css-2491253-21.patch, failed testing.

aadrian’s picture

Status: Needs work » Needs review
StatusFileSize
new2.7 KB

rerolled

Status: Needs review » Needs work

The last submitted patch, 24: move_file_admin_css-2491253-21.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chernous_dn’s picture

Status: Needs work » Needs review
StatusFileSize
new1.88 KB

Reuploading the patch from #19 I think this is the correct patch.

Status: Needs review » Needs work

The last submitted patch, 27: move_file_admin_css-2491253-19.patch, failed testing.

mayurjadhav’s picture

Status: Needs work » Needs review
StatusFileSize
new1.85 KB

I think this patch will work.

joginderpc’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new36.86 KB

Manual tested and verified file is moved to classy theme. screen shot attached below.

destination of file.admin.css

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

When changing core css files, they need to be moved to stable. Perhaps when this issue was created stable did not exist. Classy extends stable.

chernous_dn’s picture

Status: Needs work » Needs review
StatusFileSize
new1.22 KB

Update my old patch #4 rename and move to classy css file.

chernous_dn’s picture

#alexpott In stable core\themes\stable\css\file\file.admin.css already exist this file.

alexpott’s picture

Title: move file.admin.css into Classy » Remove file.admin.css from file module

@Chernous_dn slo then moving to classy is not necessary. SO just remove from core and fix the library in stable. We shouldn't need to change classy in this patch.

Status: Needs review » Needs work

The last submitted patch, 32: move_file_admin_css-2491253-31.patch, failed testing.

chernous_dn’s picture

Status: Needs work » Needs review
StatusFileSize
new922 bytes

Remove from core module file file.admin.css. .

mayurjadhav’s picture

@alexpott, do we need to create "stable.libraries.yml" in stable? else Chernous_dn's patch looks perfect.

chernous_dn’s picture

StatusFileSize
new8.76 KB

Hi @mayurjadhav if I'm not mistaken file.admin.css include in stable.info.yml attach screenshot.

mayurjadhav’s picture

Yea, correct. Patch looks perfect for me now.

manjit.singh’s picture

Status: Needs review » Reviewed & tested by the community

changes in #36 looks good to me :) moving it to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed cecc8f7 and pushed to 8.2.x. Thanks!

  • catch committed 105b856 on 8.2.x
    Issue #2491253 by Chernous_dn, rudraram, mortendk, pjbaert, LewisNyman,...

Status: Fixed » Closed (fixed)

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