Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff.txt | 790 bytes | rteijeiro |
#39 | 1862140-edit-css-organization-39.patch | 11.44 KB | rteijeiro |
#39 | interdiff.txt | 790 bytes | rteijeiro |
#38 | 1862140-edit-css-organization-38.patch | 19.14 KB | ainz |
#32 | 1862140-edit-css-organization-32.patch | 19.14 KB | ainz |
Comments
Comment #1
Wim LeersComment #2
Wim Leers.
Comment #3
Wim Leers.
Comment #4
rteijeiro CreditAttribution: rteijeiro commentedShould I use the naming conventions described in CSS file organization (for Drupal 8) instead?
Comment #5
Wim LeersYes :)
Comment #6
rteijeiro CreditAttribution: rteijeiro commentedUpdated title and description.
Comment #7
rteijeiro CreditAttribution: rteijeiro commentedMy 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.
Comment #8
jessebeach CreditAttribution: jessebeach commentedrteijeiro, 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:
Comment #9
rteijeiro CreditAttribution: rteijeiro commentedHi 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)
Comment #10
rteijeiro CreditAttribution: rteijeiro commentedAnother patch try with new naming and the file for the icons styles.
Comment #11
oresh CreditAttribution: oresh commented@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)?
Comment #12
rteijeiro CreditAttribution: rteijeiro commentedHope I fixed the IE gradients.
Also I added the -moz and -webkit prefixes to the box-shadow and border-radius styles.
Comment #13
rteijeiro CreditAttribution: rteijeiro commentedSorry, 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.
Comment #14
jessebeach CreditAttribution: jessebeach commentedReal 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?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.Comment #15
rteijeiro CreditAttribution: rteijeiro commentedThanks Jesse, I hope now the patch is right.
Comment #16
ainz CreditAttribution: ainz commentedHi 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.
Comment #17
rteijeiro CreditAttribution: rteijeiro commentedHi, @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 ;)
Comment #18
ainz CreditAttribution: ainz commentedThanks @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
Comment #20
ainz CreditAttribution: ainz commentedOk 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?
Comment #21
ainz CreditAttribution: ainz commentedUPDATE: This patch is wrong again! Ummm...still getting used to patching.
Ok so I believe this one is both of our changes together.
Comment #22
ainz CreditAttribution: ainz commentedOk, if I get this wrong again I'll be embarassed. Here goes! This is #17 and #18 together
Comment #24
ainz CreditAttribution: ainz commented#22: 1862140-edit-css-organization-22.patch queued for re-testing.
Comment #26
Wim LeersNo need to be embarrassed. Working with patch files is tricky at first, but soon enough it'll become second nature :)
Comment #27
ainz CreditAttribution: ainz commented:) Thanks for the encouragement @Wim Leers
Comment #28
droplet CreditAttribution: droplet commentedDrop 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
Comment #29
rteijeiro CreditAttribution: rteijeiro commented- Deleted -moz prefixes.
- Deleted one MS Filter.
- Deleted all Transitions prefixes.
Hope it's right now :)
Comment #30
droplet CreditAttribution: droplet commentedSorry I forgot to tell Safari and most andriod mobile needed -webkit-transitions
wonder why it don't use a .class for same functionality and styling element
http://drupal.org/node/1290494
Comment #31
rteijeiro CreditAttribution: rteijeiro commented- 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?
Comment #32
ainz CreditAttribution: ainz commentedHere 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:
Here's what I did in it:
Comment #34
ainz CreditAttribution: ainz commentedOk 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!
Comment #35
ainz CreditAttribution: ainz commentedHey 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 :)
Comment #36
droplet CreditAttribution: droplet commented#32: 1862140-edit-css-organization-32.patch queued for re-testing.
Comment #37
droplet CreditAttribution: droplet commentedI 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:
there're so many multiple class rules.
why can't just use .edit-editable, .edit-highlighted,...etc
underscore in #id ?
doesn't it inherited already ?
khtml prefix killed.
any .blank-button / .blue-button.. used in non "button" elements ?
Comment #38
ainz CreditAttribution: ainz commentedOk, 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:
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.
Comment #39
rteijeiro CreditAttribution: rteijeiro commentedI 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.
Comment #40
rteijeiro CreditAttribution: rteijeiro commentedSorry, the interdiff is reversed :)
This is the right one.
Comment #41
ainz CreditAttribution: ainz commented@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:
Comment #42
rteijeiro CreditAttribution: rteijeiro commentedHi @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 ;)
Comment #43
ainz CreditAttribution: ainz commentedThanks @rteijeiro. :)
Comment #44
Wim Leers#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.
Comment #45
Wim LeersComment #45.0
Wim LeersUpdating issue description