Clean up editor CSS inline with our CSS standards

CSS lint is clean except usage of !important. Don't know how the removal impacts.

This is what !important doing :) @lewis how will we tackle this

For #2.

Working on it...

removing width: 300px from core/assets/vendor/jquery.ui/ui/dialog-min.js also remove css as per suggestion in #5

We can't touch vendor files. Revert the change to jquery.ui/ui/dialog-min.js please.

Thanks nod_
@lewis can you suggest a better approach of this to do, rather than changes in core/assets/vendor/jquery.ui/ui/dialog-min.js

Looking into this.

I suppose we can avoid the use of the 'important by defining a min-width and max-width instead of trying to overwrite the width defined by the vendor.
Will be trying it out.

Alright, min and max-widths are not very combinable with a inline css styling without using !important. (So we shouldn't be following my suggestion in #10)

A better and more robust approach: Use the widget the way it is supposed to be used and define the widths-restrictions with attributes inside the widget: https://api.jqueryui.com/dialog/#option-maxWidth for example.

Looking into this.

So following some assumptions on how the !important should be removed.
1) We shouldn't hack vendor files, so resetted that.
2) The width: 80% !important was defined inside core/modules/editor/css/editor.css so the width would have been 80% on desktop, non-theme-specific.
3) Reading #5 + assuming the width of 80% is the width we want for desktop (and 95% width on small screens) => if should define the width in the options of the module at 95% and let the theme take care of the max-width property on its own breakpoints etc.
4) If we don't supply a value for the jquery-ui-dialog, the default value is 300px, which is silly to overwrite the widht-property with css ( https://api.jqueryui.com/dialog/#option-width )

I found where the width should be defined for these dialogs. The only purpose of the theme's css is then to define what the max-width should be of each dialog. The only question I have with this patch is: If we decide to take this approach and make the default width 95% for these dialogs, and just defining the max width in themes, shouldn't we then just supply the width of 100% so this issue doesn't get re-openened the next time someone wants to make this 96% or higher?

One of the latest 50 commits on drupal core made the patch non-apply-able, so rerolled.
(doing exactly the same as in #12 so no need for interdiff)

#17++

This is a latest re rolled patch!

#19: "Needs reroll" does not necessarily mean "apply it and upload it". It also means "needs latest feedback addressed". See #17.

Updated the patch in #19 to include comments in #17 removed full width as well. I have attached the screenshot for the reference.

I have removed the css from the module but to include it to themes like seven and bartik we should have different issues for them.

Added the css to Seven and Bartik, so we could iterate on that.

If we use the .ui-dialog--narrow-class, shouldn't we be using the .ui-dialog class too on this element?

.

Going to have a go at changing the classes to match Drupal's CSS coding standards.

I have left the class .ui-dialog--narrow intact, as the component class of .ui-dialog is already there, so the modifier makes sense. I have also brought the CSS changes into the parent theme classy, as then it is inherited down to the sub themes.

The inline width that jquery places on ui-dialog forces seven to use important statements like:

@media all and (max-width: 48em) { /* 768px */
.ui-dialog {
width: 92% !important;
}
}

I will work on that trying to switch the width to auto and use min and max-width for setting ui-dialog's width.

don't mind me

I have worked on the patch with iro and also discussed it with MathieuSpil. This is what we did:

Replaced the editor-dialog class with the ui-dialog--narrow.
Removed the editor-dialog class from the tests, since it is not used anymore.
Moved the editor's module css to classy.
Added a default width of auto to ckeditor.js so it will be easier for themes to override it using max and min-width.
Replaced the important declaration of seven theme with min and max width.

double post

Adding screenshots from all themes affected.
There are no user interface changes as intended.

1. Seven wide before patch:

2. Seven wide after patch:

3. Seven narrow before patch:

4. Seven narrow after patch:

5. Bartik wide before patch:

6. Bartik wide after patch:

7. Bartik narrow before patch:

8. Bartik narrow after patch:

9. Classy wide before patch:

10. Classy wide after patch:

11. Classy narrow before patch:

12. Classy narrow after patch:

Nice work on the screenshots there @iro! There appears to be no visual regression in the screenshots, and the new CSS is much easier to override and much cleaner. RTBC+1 from me.

@iro: Thanks for the visual regression testing!

+++ b/core/themes/classy/css/components/ui-dialog.css
@@ -0,0 +1,14 @@
+ * Styles for Classy's modal windows

Nit: missing trailing period. Fixed that silly typo myself.

Given that #36 shows there are no visual regressions, and the patch in #34 implements exactly what @LewisNyman asked in #17, and I only fixed a typo here, I think I can RTBC this.

Good work here @kostask and @iro!

@kostask: If you work on a ticket where there is already a history of patches, your explanation in #34 can be clarified + checked by supplying an interdiff. Like so:https://www.drupal.org/documentation/git/interdiff

RTBC +1

Damn it! @LewisNyman outnitpicked me :P

I will add the blank line as indicated by #40.

Here is the patch.
@wiifm, @Wim Leers, @MathieuSpil: Thank you for your kind words.

Oops, forgot to change status.

Since all comments are addressed I am RTBCing that one.

@MathieuSpil thanks for the tip!

Looks like the bot failed randomly (error was "

fail: [Completion check] Line 78 of core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php:
The test did not complete due to a fatal error.

")

Now it is a legitimate failure, due to #2566771: [Voltron patch] Move all remaining *.theme.css to Classy having been committed yesterday.

Here is the patch rerolled

PIFR is drunk.

This looks like a good cleanup of editor's CSS and thanks for all the screenshots. Committed b8a6e72 and pushed to 8.0.x. Thanks!

This removed editor.css but didn't remove it from the library definition. Here's a small follow-up: #2587563: editor.libraries.yml references editor.css which has been removed