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.

Files: 
CommentFileSizeAuthor
#40 interdiff.txt790 bytesrteijeiro
#39 1862140-edit-css-organization-39.patch11.44 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 55,374 pass(es).
[ View ]
#39 interdiff.txt790 bytesrteijeiro
#38 1862140-edit-css-organization-38.patch19.14 KBainz
PASSED: [[SimpleTest]]: [MySQL] 55,420 pass(es).
[ View ]
#32 1862140-edit-css-organization-32.patch19.14 KBainz
PASSED: [[SimpleTest]]: [MySQL] 55,407 pass(es).
[ View ]
#31 1862140-edit-css-organization-31.patch11.29 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 54,776 pass(es).
[ View ]
#29 1862140-edit-css-organization-29.patch9.78 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 54,770 pass(es).
[ View ]
#22 1862140-edit-css-organization-22.patch11.95 KBainz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862140-edit-css-organization-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 1862140-edit-css-organization-21.patch9.22 KBainz
PASSED: [[SimpleTest]]: [MySQL] 54,806 pass(es).
[ View ]
#18 1862140-add-css-paths-18.patch475 bytesainz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862140-add-css-paths-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 1862140-edit-css-organization-17.patch11.85 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 54,965 pass(es).
[ View ]
#15 1862140-edit-css-organization-15.patch11.43 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 54,834 pass(es).
[ View ]
#13 1862140-edit-css-organization-13.patch11.77 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 54,758 pass(es).
[ View ]
#12 1862140-edit-css-organization-12.patch11.64 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 54,870 pass(es).
[ View ]
#10 1862140-edit-css-organization-10.patch8.83 KBrteijeiro
FAILED: [[SimpleTest]]: [MySQL] 54,434 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#7 1862140-edit-css-organization-7.patch5.5 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 54,542 pass(es).
[ View ]

Comments

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.

Issue tags:+Novice, +Spark

.

Component:other» edit.module

.

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

Yes :)

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.

Status:Active» Needs review
StatusFileSize
new5.5 KB
PASSED: [[SimpleTest]]: [MySQL] 54,542 pass(es).
[ View ]

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.

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

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)

StatusFileSize
new8.83 KB
FAILED: [[SimpleTest]]: [MySQL] 54,434 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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)?

StatusFileSize
new11.64 KB
PASSED: [[SimpleTest]]: [MySQL] 54,870 pass(es).
[ View ]

Hope I fixed the IE gradients.

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

StatusFileSize
new11.77 KB
PASSED: [[SimpleTest]]: [MySQL] 54,758 pass(es).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new11.43 KB
PASSED: [[SimpleTest]]: [MySQL] 54,834 pass(es).
[ View ]

Thanks Jesse, I hope now the patch is right.

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.

StatusFileSize
new11.85 KB
PASSED: [[SimpleTest]]: [MySQL] 54,965 pass(es).
[ View ]

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 ;)

StatusFileSize
new475 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862140-add-css-paths-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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?

StatusFileSize
new9.22 KB
PASSED: [[SimpleTest]]: [MySQL] 54,806 pass(es).
[ View ]

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

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

StatusFileSize
new11.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862140-edit-css-organization-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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

:) Thanks for the encouragement @Wim Leers

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

Status:Needs work» Needs review
StatusFileSize
new9.78 KB
PASSED: [[SimpleTest]]: [MySQL] 54,770 pass(es).
[ View ]

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

Hope it's right now :)

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

StatusFileSize
new11.29 KB
PASSED: [[SimpleTest]]: [MySQL] 54,776 pass(es).
[ View ]

- 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;
}

StatusFileSize
new19.14 KB
PASSED: [[SimpleTest]]: [MySQL] 55,407 pass(es).
[ View ]

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.

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!

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 :)

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 ?

StatusFileSize
new19.14 KB
PASSED: [[SimpleTest]]: [MySQL] 55,420 pass(es).
[ View ]

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.

StatusFileSize
new790 bytes
new11.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,374 pass(es).
[ View ]

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.

StatusFileSize
new790 bytes

Sorry, the interdiff is reversed :)

This is the right one.

@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.

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 ;)

Thanks @rteijeiro. :)

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.

Issue tags:-sprint

Issue summary:View changes

Updating issue description