Problem/Motivation

Although relatively clean and well structured, the code inside edit.css needs to have the structure styles separated from the presentation styles.

RTL must be taken into account as well.

Proposed resolution

Nothing more than doing the cleanup work following this conventions: http://drupal.org/node/1887922

Remaining tasks

Do it.

User interface changes

Should be no UI changes.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: Break Edit modules edit.css into the proper .base and .theme files. Address RTL styling as well. » Break Edit module's edit.css into the proper .base and .theme files. Address RTL styling as well.
Wim Leers’s picture

Issue tags: +Novice, +Spark

.

Wim Leers’s picture

Component: other » edit.module

.

rteijeiro’s picture

Should I use the naming conventions described in CSS file organization (for Drupal 8) instead?

Wim Leers’s picture

Yes :)

rteijeiro’s picture

Title: Break Edit module's edit.css into the proper .base and .theme files. Address RTL styling as well. » Break Edit module's edit.css into the proper files. Address RTL styling as well.
Assigned: Unassigned » rteijeiro

Updated title and description.

rteijeiro’s picture

Status: Active » Needs review
FileSize
5.5 KB

My first attempt of patch. Probably I should move the icon styles to the skin file but I am not sure. Tell me what do you think about that.

jessebeach’s picture

rteijeiro, you're a coding beast! How do you have so much time?!? I love it.

These are the file name patterns we've been using:

{some}-{name}.base.css
{some}-{name}.base-rtl.css
{some}-{name}.theme.css
{some}-{name}.theme-rtl.css
{some}-{name}.icons.css
{some}-{name}.icons-rtl.css
rteijeiro’s picture

Hi Jesse, I am afraid I have lost your webinar today, hope everything went well.

Actually I am far, far away form my family, friends and other temptations for getting fun so I try to get fun in the issue queue :P

In reference of the issue, then I should not follow the naming conventions of the issue [#1887922], is it right?

[ Edit: In this issue they are following these conventions for other modules http://drupal.org/node/1925320 ]

(I don't know what happen today with issue numbers that not become into links)

rteijeiro’s picture

Another patch try with new naming and the file for the icons styles.

oresh’s picture

Status: Needs review » Needs work

@rteijeiro, nice! )
But.. hm.. by the time Drupal comes out, i'm not sure we would need to use -o- or -ms- prefixes.
Also could please add IE support for opacity and gradient (example of gradients ie supports)?

rteijeiro’s picture

Hope I fixed the IE gradients.

Also I added the -moz and -webkit prefixes to the box-shadow and border-radius styles.

rteijeiro’s picture

Sorry, forgot the opacity styling.

Also I don't know how to fix opacity in transitions for IE. I guess it's not possible for IE versions until IE10.

The other patch fails due a error of the testbot. The patch is right.

jessebeach’s picture

Real quick, we don't need to bother with polyfilling gradients for older versions of IE. Let's stick with the standard CSS3 gradients linear-gradient. The MS filters destroy browser performance. Just make sure you have a background-color set as a default and then set a gradient on top that goes from transparent to a somewhat-transparent white or black -- no color in the gradient, to create an overlay on a solid color. This way we can change the base backround-color but not need to change the gradient and browsers that don't support the gradient just get a solid color. Make sense?

+#edit_modal button.gray-button,
+.edit-toolbar button.gray-button {
+  color: #666;
+  background-image: -webkit-linear-gradient(top, #f5f5f5 0%, #ccc 100%);
+  background-image: -moz-linear-gradient(top, #f5f5f5 0%, #ccc 100%);
+  background-image: linear-gradient(top, #f5f5f5 0%, #ccc 100%);
/* No filters, please. They're terrible for performance */
+  filter: progid:DXImageTransform.Microsoft.gradient(startColorstr='#f5f5f5', endColorstr='#ccc');
+  -moz-border-radius: 5px;
+  -webkit-border-radius: 5px;
+  border-radius: 5px;
+}

Also, -webkit-linear-gradient is the only prefixed linear-gradient we need for older Webkits on phones. Every other browser supports the standard or doesn't support gradients at all.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
11.43 KB

Thanks Jesse, I hope now the patch is right.

ainz’s picture

Hi there I'm very new to contributing so please bear with me. @rteijeiro I downloaded your patch and applied it and I can see it in my core/modules/edit/css directory but when go to my edit page and I look for the classes and id's you added it's not in the page.

I was assuming that drupal was going to change the output of the html based on the classes and id's in the file but I guess I was wrong. How do I test this patch? Thanks for your help in understanding this.

rteijeiro’s picture

Hi, @ainz.

I forgot to update the edit.module to take the correct css file. Now it's fixed in this patch but I am not sure if this is out of the scope of this issue. We must wait for the opinion of jessebeach or someone with more experience ;)

ainz’s picture

FileSize
475 bytes

Thanks @rteijeiro! I tested it but it still wasn't loading the icons and some other stuff then I realised that two paths were missing so I created this patch to be applied after yours. This is my first patch ever so I hope I did this right. :)

NOTE: This path is to be applied after the patch in #17

Status: Needs review » Needs work

The last submitted patch, 1862140-add-css-paths-18.patch, failed testing.

ainz’s picture

Status: Needs work » Needs review

Ok so I realise the patch failed because yours needs to be applied first. So how does this work. If I find something that I'd like to fix in your patch do I add it to your patch or do I tell you and you add it? I just figured that adding it to your patch would be taking away from your credit when I submitted. So how does the community usually do it?

ainz’s picture

UPDATE: This patch is wrong again! Ummm...still getting used to patching.

Ok so I believe this one is both of our changes together.

ainz’s picture

Ok, if I get this wrong again I'll be embarassed. Here goes! This is #17 and #18 together

Status: Needs review » Needs work
Issue tags: -Novice, -Spark, -edit-followup, -CSS novice

The last submitted patch, 1862140-edit-css-organization-22.patch, failed testing.

ainz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +Spark, +edit-followup, +CSS novice

The last submitted patch, 1862140-edit-css-organization-22.patch, failed testing.

Wim Leers’s picture

No need to be embarrassed. Working with patch files is tricky at first, but soon enough it'll become second nature :)

ainz’s picture

:) Thanks for the encouragement @Wim Leers

droplet’s picture

Drop border-radius prefix, please :)
http://drupal.org/node/1290506
http://drupal.org/node/1422614

Also, only IE10 supports Transitions, and it supported un-prefixed syntax

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
9.78 KB

- Deleted -moz prefixes.
- Deleted one MS Filter.
- Deleted all Transitions prefixes.

Hope it's right now :)

droplet’s picture

Sorry I forgot to tell Safari and most andriod mobile needed -webkit-transitions

+++ b/core/modules/edit/css/edit.base-rtl.cssundefined
@@ -0,0 +1,28 @@
+#edit_modal button,
+.edit-toolbar button {

wonder why it don't use a .class for same functionality and styling element

+++ b/core/modules/edit/css/edit.theme.cssundefined
@@ -0,0 +1,41 @@
+  -khtml-opacity: 0;

http://drupal.org/node/1290494

rteijeiro’s picture

- Changed the #edit_modal id for a .edit-modal class in edit.base.css and theme.js files.
- Also added -webkit-transition.
- Deleted -khtml prefixes.

What about this?

.edit-toolbar-container {
  -webkit-user-select: none;
  -khtml-user-select: none;
     -moz-user-select: none;
      -ms-user-select: none;
       -o-user-select: none;
          user-select: none;
}
ainz’s picture

Here goes! I figured out what I did wrong yesterday, I tested this patch and it works.

I updated the edit.icons.css file with the 'edit-modal' class you changed in the last patch. Also I had to put back -moz- in the edit.icons file for the button images to load in mozilla.

UPDATE: This patch was created on git tags. I just read this in the Advanced Patch Contributor Guide:

Do not attempt to create or apply patches to git tags. This causes trouble for not only you, but everyone else trying to use your patch. Except in very specific an unusual cases, patch development should generally be done on branches.

Here's what I did in it:

  1. Changed the id's #edit_modal in the edit.icons.css file to classes .edit-modal as the colors weren't showing up on the buttons
  2. Several of the id's were still edit_modal so when I changed them to classes I updated it to edit-modal
  3. Added the -moz- property back to the background images in the file edit.icons.css because the background images weren't showing up in firefox

Status: Needs review » Needs work

The last submitted patch, 1862140-edit-css-organization-32.patch, failed testing.

ainz’s picture

Status: Needs work » Needs review

Ok this is better, just one test failed of 54,870. Can someone help me figure out why? I see that it's some JavaScript Test but I don't understand it. Thanks much!

ainz’s picture

Hey guys, if someone can point me in the right direction as to the error I'll really appreciate it. I didn't want to try it again and make the queue longer with failed patches :)

droplet’s picture

droplet’s picture

I haven't do a real test on D8. don't know where the elements hidden.

Based on the CSS styles, I wonder why to do it:

+++ b/core/modules/edit/css/edit.base.cssundefined
@@ -0,0 +1,242 @@
+.edit-field.edit-editable,
+.edit-field .edit-editable {
+++ b/core/modules/edit/css/edit.base.cssundefined
@@ -0,0 +1,242 @@
+.edit-field.edit-editable.edit-highlighted,
+.edit-form.edit-editable.edit-highlighted,
+.edit-field .edit-editable.edit-highlighted {
...
+.edit-field.edit-editable.edit-highlighted.edit-validation-error,
+.edit-form.edit-editable.edit-highlighted.edit-validation-error,
+.edit-field .edit-editable.edit-highlighted.edit-validation-error {

there're so many multiple class rules.
why can't just use .edit-editable, .edit-highlighted,...etc

+++ b/core/modules/edit/css/edit.base.cssundefined
@@ -0,0 +1,242 @@
+#edit_backstage {

underscore in #id ?

+++ b/core/modules/edit/css/edit.base.cssundefined
@@ -0,0 +1,242 @@
+.edit-form .form-wrapper { margin: .5em; }
+.edit-form .form-wrapper .form-wrapper { margin: inherit; }

doesn't it inherited already ?

+++ b/core/modules/edit/css/edit.base.cssundefined
@@ -0,0 +1,242 @@
+   -khtml-user-select: none;

khtml prefix killed.

+++ b/core/modules/edit/css/edit.icons.cssundefined
@@ -0,0 +1,57 @@
+.edit-toolbar button.blank-button {
...
+.edit-modal button.blue-button,
+.edit-toolbar button.blue-button {
...
+.edit-modal button.gray-button,
+.edit-toolbar button.gray-button {
...
+.edit-modal button.blue-button:hover,
+.edit-toolbar button.blue-button:hover,
+.edit-modal button.blue-button:active,
+.edit-toolbar button.blue-button:active {

any .blank-button / .blue-button.. used in non "button" elements ?

ainz’s picture

Ok, this patch was made against the branch as opposed to the git tag (used in patch submitted in #32) as outlined in the Advanced Patch Contributor Guide. I tested this and it works!

The changes made were:

  1. Changed the id's #edit_modal in the edit.icons.css file to classes .edit-modal as the colors weren't showing up on the buttons
  2. Several of the id's were still edit_modal so when I changed them to classes I updated it to edit-modal
  3. Added the -moz- property back to the background images in the file edit.icons.css because the background images weren't showing up in firefox

Is it just me or does anyone else notice that the appearance of the quick edit contextual menu is random. I'm using Firefox 20 on ubuntu and sometimes I won't get it but then when I refresh or visit the page from the homepage I'll get it. Weird.

rteijeiro’s picture

I guess probably we must preserve the id and add the class in theme.js, as I have seen in other issues.

So I created the patch for that and the interdiff.

rteijeiro’s picture

FileSize
790 bytes

Sorry, the interdiff is reversed :)

This is the right one.

ainz’s picture

@rteijeiro I'm just curious as I'm new to the whole patching thing. How is it that even though your patch file has everything my patch file has in it and more, mine is 19kb and yours is 11kb? Also I noticed that I always have dev/null in my patch file but yours doesn't. Am I doing something that I shouldn't?

Which issue are you referring to when you say:

I guess probably we must preserve the id and add the class in theme.js, as I have seen in other issues.

rteijeiro’s picture

Hi @ainz,

The answer to the first question: http://drupal.org/node/1542048

The answer to the second question: I don't remember exactly where I have seen that, but if you take a look to similar issues in http://drupal.org/node/1921610 you will see that people don't remove the id from the html but they add the class in the html and then change the css. Hope to explain it well.

Feel free to ask. That's the way to learn ;)

ainz’s picture

Thanks @rteijeiro. :)

Wim Leers’s picture

Status: Needs review » Closed (fixed)
Issue tags: -Novice +sprint

#1678002: Edit should provide a usable entity-level toolbar for saving fields covered all of this AFAICT! :)

Please double-check and reopen if I'm wrong.

Wim Leers’s picture

Issue tags: -sprint
Wim Leers’s picture

Issue summary: View changes

Updating issue description