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.
Part of the CSS Cleanup: http://drupal.org/node/1089868
Overview of Goals
- Make it easy to remove unwanted design assumptions in the theme layer, while maintaining critical functionality (such as functional JavaScript widgets).
- Prevent uneeded administrative styles from loading on the front end.
- Give modules the ability to include a generic design implementation with their module, without burdening themers.
- Make CSS and related markup more efficient and less intrusive to improve the themer experience.
The CSS Clean-up Process
Use the following guidelines when writing patches for the core issues listed below.
- Put CSS is in the appropriate file: CSS should be moved to separate files, using the following
name spacing conventions based on their purpose:- module.base.css
- Should hold structural and behavior related styling. CSS should be coded against the Stark theme. The absolute bare minimum CSS required for the module to function should go here. If there is no CSS required, this file should be omitted.
- module.theme.css
- Should hold generic design-related styles that could be used with Stark and other themes. It's where all design assumptions like backgrounds, borders, colors, fonts, margins, padding, etc, would go.
- module.admin.css
- Should hold styles that are only applicable to administrative pages.
To see an example of this in practice, look at Drupal's system module.
- Remove Assumptions: Styles that make too many assumptions, introduce superflous margins, padding and add things like font settings are not necessary and don't belong in core module CSS files. In cases where core themes depend on these properties, they should be moved to the CSS stylesheet of the respective theme.
- Reduce Selector Specificity: CSS code that resides in modules should be written in a way that's easily overridable in the theme layer. To improve the Themer Experience and make core CSS more efficient, CSS selectors should be made as general and short as possible. For example:
- Use
.style {}
overdiv.style {}
where possible. - Use
.module .style {}
overdiv.module div.somenestedelement .style
where possible.
- Use
- Don't use IDs in selectors: Use of ID's in core CSS selectors requires more specificity in the theme layer, making it harder and more annoying to deal with. It makes achieveing consistency in complex design implementations much harder than it needs to be. We need to stop making life hard for theme developers.
- Don't be afraid to change markup: There's lots of overlap between using proper and semantic markup and doing CSS right. If you come across a case where CSS is being applied where using a more semantic elements would solve the problem, then change the markup in your patch to make it right. For more information, see the Drupal 8 Markup Gate rules.
- Start with Stark and cross-browser test.
- "Design" markup and CSS for the Stark theme.
- If applicable, adapt the styles to match the core themes afterward.
- Finally, test the changes in all supported browsers and ensure no regressions are introduced.
Comment | File | Size | Author |
---|---|---|---|
#132 | 1216978-contextual_module-css-clean-up-132.patch | 3.99 KB | oresh |
#118 | 1216978-118.patch | 11.14 KB | Jacine |
#112 | 1216978-112-contextual.patch | 11.28 KB | cosmicdreams |
#110 | 1216978-110-contextual.patch | 11.28 KB | cosmicdreams |
#109 | new_patch.patch | 6.92 KB | cosmicdreams |
Comments
Comment #1
bleen CreditAttribution: bleen commentedplease note: #1290562: Contextual: remove duplicated css
Comment #2
jessebeach CreditAttribution: jessebeach commentedsubscribe
Comment #2.0
jessebeach CreditAttribution: jessebeach commentedCorrecting link to CSS Cleanup
Comment #3
cosmicdreams CreditAttribution: cosmicdreams commentedStarting in on this process, likely will make some mistakes as this is my first attempt to clean up the CSS. But I'm following the guidelines above.
Comment #4
cosmicdreams CreditAttribution: cosmicdreams commentedFirst attempt: This patch does the following
I tested all of these changes in IE7, IE8, IE9, and Chrome (canary) using Stark. I'll go back to testing the rest of the browsers I have access to later tonight.
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commentedSorry, this one's a better patch. Used git format-patch to make this one.
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like I submitted the patch in the wrong format. Here's another attempt, this time using better syntax.
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedI should also note that the above patch doesn't strip the contextual links module of it's current look and feel. After re-reading 2. Remove Assumptions instruction, I could have done more to strip out the extra formatting that is present in this module.
Curved corners, padding for the li and links, are possibly more extraneous formatting are not needed for Contextual links to be functional even though it makes it more user friendly. If I'm reading the instruction for 2 correctly, I should remove anything that imposes a style or design choice. In Stark it should merely be functional.
Comment #9
TR CreditAttribution: TR commented@cosmicdreams: Patch is 0 bytes ...
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedThis one is not empty
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedset to review so bot can test patch. Looks like the previous patch didn't include the new code, only removed the old code. I'll need to redo it again.
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commentedThis next patch is verbose, but it does the job of removing the contents of one file and adding the renamed file. I've tried to use git format-patch -C -M --stdout > mypatch.patch so many times and I don't know why that isn't working.
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedNeed to provide a proper patch that has to goal of :
Comment #14
jyve CreditAttribution: jyve commentedHi cosmicdreams,
Have you followed the steps in http://drupal.org/node/1054616? They work for me.
I will chip in with my attempt to a patch. Hopefully I have more luck :)
What to find in the patch:
Comment #15
cosmicdreams CreditAttribution: cosmicdreams commentedHi jyve:
Thanks for the effort, I was going to take another try at this during the weekend. I can share a few notes with you for guidance on what they're looking for in this patch:
Create a module.base.css that includes all of the styles that make this feature work.
Create a module.theme.css that only makes the feature look exactly like it did before.
In this patch changes like including !important to the styles would not be advised for two main reasons:
1. The styles would end up being different than the original styles. Make large changes to the styles are not in scope
2. Adding !important would make it more difficult for other modules to override the style of the contextual menu.
So with those details in mind. We should make the patch as slim as possible while focus on refactoring the existing styles into the base.css and the styles that could potentially NOT be loaded and everything would be fine. Does that make sense?
Comment #16
jyve CreditAttribution: jyve commentedI totally agree with the fact that css for core modules should be easy to overwrite. However, I do believe that contextual links is a bit of an exception since it the HTML for this module is mixed in with all other fronted HTML. If we make it too easy to overwrite, a lot of people will see their contextual links layout break whenever they add styling to their .item-list .node-article li for example. So the !important to me just makes sure that the layout of these links don't break. They can still be overwritten if someone wants to do so.
For the base/theme css file, my feeling is that the module works without any styling, since the links are printed at the top of the entity that you can edit. All css that has been added, simply add nice styling, so should be in theme.css?
Comment #17
JacineThis is actually the precise reason to have a
*.base.css
. I agree that Contextual links are somewhat of a special case, but there's no reason we can't do this smarter. IMO, the layout aspects of this belong incontextual.base.css
(we can make them as strong as they need to be so they're not easily broken), and then all the colors and other crap should go incontextual.theme.css
.The idea is that if we want to change the look of it in our themes, we don't have to loose all future layout fixes. We can just remove
contextual.theme.css
and easily provide our own presentation code. A good example of this can be found in page 2 of this PDF (Collapsible fieldsets): http://bit.ly/cshEyLAnd once we're done with this throughout core, a base theme can remove all the
*.theme.css
files and get a fantastic starting point without breaking JavaScript functionality or having so many preset presentational aspects. Does that make sense?Comment #18
jyve CreditAttribution: jyve commentedApparently the base vs theme css was not really clear to me yet, thanks for clarifying! Attached is a new patch that seperates functionality (base.css) from style (theme.css). I've removed all the !importants from the previous patch, but did make the selectors on the ul and li slightly more specific to compensate :).
All feedback is welcome.
Comment #19
JacineThanks @jyve! Just took a quick glance and this is looking great! :D
Found a couple of minor code style issues. Will try to test more later!
Needs an empty line at the bottom of the file.
Indentation needs work.
-14 days to next Drupal core point release.
Comment #20
jyve CreditAttribution: jyve commentedNew patch attached to fix the formatting issues @Jacine mentioned.
Comment #21
cosmicdreams CreditAttribution: cosmicdreams commented#20 is a solid patch. When I test against stark and delete the contents of the contextual.theme.css the feature still functions. I don't get the fancy gears or anything but the module isn't broken. Exactly the aim of this issue.
I recommend RTBC.
Comment #22
chx CreditAttribution: chx commentedThere's only CSS in there so it doesnt need tests , either.
Comment #23
JacineI agree the patch does look good, but haven't tested it and one person testing isn't enough yet. Let's not rush. This RTBC is coming very soon, don't worry. In the meantime, can we get a couple of screenshots - one with the contextual.theme.css and one with just contextual.base.css?
Thanks!
Comment #24
kappaluppa CreditAttribution: kappaluppa commentedComment #25
jyve CreditAttribution: jyve commentedAttached is a screenshot of the contextual links with and without the theme.css.
Comment #26
cosmicdreams CreditAttribution: cosmicdreams commentedThat's great jyve, but can you submit set of screenshots that demonstrate the with and without contextual.theme.css using the Stark theme. We're supposed to base our refactoring work off of Stark, not Bartik.
If I have time tonight, I might beat you to it.
Comment #27
jyve CreditAttribution: jyve commentedNew screenshot attached, in the Spark theme this time.
Comment #28
cosmicdreams CreditAttribution: cosmicdreams commentedThanks Jyve. In addition to this screenshot, seeing the patch in play is really illustrative. Everything works properly without the contextual.theme.css. Further optimizations could happen in future patches.
Comment #29
bleen CreditAttribution: bleen commentedshouldnt the fact that the links are positioned right be part of the theme.css file?
Comment #30
jyve CreditAttribution: jyve commentedThe positioning to me is about good usability, not about styling so I believe it belongs in the base.css
Comment #31
bleen CreditAttribution: bleen commentedI disagree ... by putting the positioning rules in contextual.base.css instead of contextual.theme.css you are making a decision about what is most usable NOT what is the minimum amount of css needed to make these links function properly.
Consider the perfectly valid case of the site that wants their contextual links to be positioned on the bottom left of all blocks (etc...). Why is that unusable?
If others disagree, I will not press any further ...
Comment #32
jyve CreditAttribution: jyve commentedOk, you make a valid point there. My opinion is partly based on the fact that when looking at other base.css files, I noticed that positioning is considered 'basic behaviour'.
Let's see if we can get more opinions on this :)
Comment #33
xjmI'd agree that the positioning probably does not belong in the
.base.css
in this case.Tagging as Needs Manual Testing to get a few more people testing the changes as Jacine recommends in #23. (All these CSS cleanup issues probably should have that tag, actually.)
Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #34
jyve CreditAttribution: jyve commented3 changes in this new patch:
1. Rerolled the patch to match the new Drupal folder structure.
2. Moved the positioning from base.css to theme.css, as rightfully mentioned in #29.
3. Added @file headers.
A new screenshot is attached.
Comment #35
xjmOops, some trailing whitespace here.
Other than that, the patch code style looks good, and the updated screenshots look good as well. Let's have a couple people test it and maybe review the CSS to check whether there's anything that could be optimized.
Comment #36
jyve CreditAttribution: jyve commentedtrailing white-space removed.
Comment #37
aspilicious CreditAttribution: aspilicious commentedcss should be placed in an alphabetical order.
Hmm I'm not sure Jacine will like this like jyve noticed told me before to not change the behaviour/looks to much in stark.
She needs to shim in. Adding sprint tag for that.
18 days to next Drupal core point release.
Comment #38
cosmicdreams CreditAttribution: cosmicdreams commentedWhy are we even bothering with browser specific border radius?
http://caniuse.com/#search=border-radius says the only browsers we have to be concerned with not supporting are IE8 and 7 and there's now way to get curved corners to work performantly for those.
If you keep in mind that Drupal 8 is targetting future browers and I think you'll see how much a waste of space browser specific curved corners are.
Comment #39
sunThe styles are in alphabetical order already. See #746768: Determine coding standards for CSS3 properties for more information on CSS3 and browser-specific properties.
Patch looks good, but I didn't test it.
Comment #40
aspilicious CreditAttribution: aspilicious commentedThen we are not very consistent if the specific border stuff needs to be in the end
17 days to next Drupal core point release.
Comment #41
aspilicious CreditAttribution: aspilicious commentedNeeds review...
Comment #42
droplet CreditAttribution: droplet commentedThere is a issue related to border-radius:
#1290506: Remove webkit-specific border radius from CSS
Comment #43
jyve CreditAttribution: jyve commentedWhile I am pro removing all browser specific css (-webkit and -moz), I would prefer to await the conclusion of the discussion in #1290506: Remove webkit-specific border radius from CSS and then update all core css as a separate task.
The goal of this issue is not related to that, and should not be slowed down by it.
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedAgree this patch test (manually) good RTBC++
Comment #45
mortendk CreditAttribution: mortendk commentedokay this is maybe a new issue but is this really not rtbc were kinda forgetting a lot if the stuff with the cleanups that we wanna do in D8 ?
Im probably gonna be beaten with a stick now, anyways here goes
So I rolled a patch not to hijack, but it was a bit quicker this way with all the cross browser testing.
* Remove div.foo
as the guidelines says we should remove the div.foo naming & generally look at the classnames & markup so gets an overhall of fresh eyes.
* readability
the classnames is now shorter so the wrapper is now contextual-links which makes the css a hellof a lot more readable
Theres only one spot left where theres a div.class and thats the div.contextual-links ul.links, caused by bartiks idea of using .block ul as a basic
Theres one "huge" problem and thats the hardcoded prefix / suffix - i mean hardcoded markup we cant change in the theme wtf ;)
maybe is should be wrapped with theme_container() instead so we at least can touch it ?
Browser vendor prefixes
supporting that old versions of safari & khtml really dont make any sense - so lets cut the crap now & remove the vendor prefixe's - contextual links would be a good place to start ;)
Im sorry about this not sure if it should go in as a new issue (im still new at this core stuff)
btw are we there yet where html.js is not nessesary and we can use .js instead ?
Comment #46
cosmicdreams CreditAttribution: cosmicdreams commented@mortendk I mostly agree and I'm totally with you on the topic of squashing the browser specific prefixes. However there is a seperate issue for that: #1290506: Remove webkit-specific border radius from CSS
Should we trim the fat during this cleanup effort or commit mass genocide on all prefixes in that issue?
Comment #47
mortendk CreditAttribution: mortendk commented@cosmicdream Well i would like that we killed it right now and take on the others - im gonna wave in for a kill browser prefixes for border-radius in the other issue. the good thing here is that we have a real issue not just an discussion ;)
Comment #48
cosmicdreams CreditAttribution: cosmicdreams commentedThen I'll advance the issue to let the bot test the most recent patch.
Comment #49
mortendk CreditAttribution: mortendk commentedfair enough -moz should not be dropped yet for the 6-7% firefox 3.6 users so its added in again
Comment #50
aspilicious CreditAttribution: aspilicious commentedprefix stuff needs to come before the real border radius stuff
trailing whitespace
Same and our current standards place properties that have prefix variants at the end
13 days to next Drupal core point release.
Comment #51
mortendk CreditAttribution: mortendk commentedto be an even better boy towards the 1% with safari4 based browsers the vendor prefix -webkit crap for each corner is added again untill theres a consensus in the development about removing support for that kinda nonsens ;)
#1344520: Drop support for safari4 in core
Comment #52
jyve CreditAttribution: jyve commentedFeedback on the last patch:
- My only concern with generalizing the HTML and CSS is that it will break more easily when theming. On the other hand, your changes seem logic, so maybe it is up to the themers to write decent CSS so that contextual links CSS is not overwritten.
- The hardcoded prefix/suffix can be overwritten in your custom module if you want, sounds like an edge case anyway, so doesn't bother me too much.
- The browser vendor prefixes are indeed a separate issue :)
- Not sure why .js would not be enough?
Also some minor issues:
- Need to alphabetize this:
- No need to have two lines between the code blocks (same in contextual.base.css):
- A white space is missing after the selector:
- White spaces missing before the values (same in the LTR version):
Comment #53
mortendk CreditAttribution: mortendk commentedwell i dare you ;) dobble dare you to try and an accedential overwrite of it - lets kick the tires a bit and see what falls off :)
hmm but i cant overwrite it in my theme right (hence the module thing in 52) - I know its an edge case, but its kinda a principle for me that everything is ease to overwrite / figure out in the theme layer - anyways its not like we dying at it is right now ;)
anyways fixed it up & alse fixed the ul li a i kinda gave a block status for all *ups*
Comment #54
mortendk CreditAttribution: mortendk commentedreview pls
Comment #55
cosmicdreams CreditAttribution: cosmicdreams commentedTwo small changes to improve the reduction of selector specificity in this patch (which builds off of #53)
Also a question. In contextual.theme.css our selector for styling .contextual-links' ul was :
But in contextual.theme-rtl.css our selector was :
Why the difference? Should this patch's .contextual-links .links in contextual.theme.css be .contextual-links ul instead?
Comment #56
mortendk CreditAttribution: mortendk commentedi just gave it a good kick with obscure css declarations to see if i could get it to break without the use of !important and other edge cases
basically if we look at a given theme having css like this
@cosmicdreams huh what did the patch do - or was i blind ?
Comment #57
cosmicdreams CreditAttribution: cosmicdreams commentedIt turned div.contextual-links ul.links into .contextual-links .links in contextual.theme.css and
it turned div.contextual-links ul into contextual-links ul in contextual.theme-rtl.css
As per the cleanup initiative's description, we aren't supposed to have divs in these css files anymore. For example:
+div.contextual-links ul.links {
+ -moz-border-radius: 0 4px 4px 4px; /* FF3.6 */
+ border-radius: 0 4px 4px 4px;
+ left: 0;
+ right: auto;
+}
In several places you have added back divs in your css with that latest patch.
Comment #58
JacineThere's conflicts all over this. We're not making any changes here as far as I can tell, so this shouldn't be in the patch.
This isn't in the patch, but as @mortendk mentioned earlier, it's a big no-no. It's not easy to deal with #theme_wrappers
theme_container()
here because #attributes ends up getting used in boththeme_container()
andtheme_links()
. Been thinking about this, and think it might make sense to actually implementtheme_links__contextual()
to handle both the wrapper and the class of the<ul>
itself.The guidelines for what we are doing to clean up are not meant to follow as a bible, it's more general. I'm pretty sure the div.contextual is contextual, per @jyve's earlier comments in this issue, and if that's the case, then it's OK.
This is somewhat of a special case, because it is heavily mixed into both front end and back end interfaces, so we need to take special care to ensure what we do here makes sense.
Comment #59
mortendk CreditAttribution: mortendk commentedthe reason i have added in the div. ul. li. stuff is to protect the style for a themes css so you dont suddenly gets to overwrite the contual links with you own line heights backgrounds whatever.
its against the rule of killing as much crap as possible but in this case i actually thinks it makes sense
Comment #60
Jacine@mortendk So... just realized that it's a .rej file. Just delete it.
Comment #61
mortendk CreditAttribution: mortendk commentedhep here it is
Comment #62
mortendk CreditAttribution: mortendk commentedthe links class that was added to the ul is now removed cause we dont need it
I would actually make a case that we keep the classes that are attached to the li - these could be used for some pretty sweet icon interface love ;)
i have tested the css up against: - but i still dare ya to bang on it, so we dont end up with a contextual linkes beeing accedential overweritten by a module
Comment #63
TR CreditAttribution: TR commentedGo, bot, go !
Comment #64
droplet CreditAttribution: droplet commenteddoes it a mistake? Yellow in color, looks very strange.
9 days to next Drupal core point release.
Comment #65
mortendk CreditAttribution: mortendk commentedRenamed the classes to make em a tiny bit shorter and keep with the exsiting namespacing and use the module name as base class name
*contextual-region
class added to the region around the contextual)
*contextual
the class for the div surround the contextual links
*contextual-links
class for the for the ul so we dont end up overweiting it with the ul.links inside a node
moved the text-decoration: none; out of .base.css its pure presentational and should not ever never then be in base.css moved to theme.css
Q's about base.css
base.css now have the position (out to the right on every region) so a new theme dont have to define that everytime
wonder how others feels about it
pro: its a basic drupal think to have the config of a region out to the right
con: dude base.css shoudnt do crap unless its absolutely nessasary
Comment #67
jessebeach CreditAttribution: jessebeach commentedRe: base.css, I think it should keep the right-positioning. It's styling that ends up enabling the basic functionality.
Love this! This module really really needed this type of attention and overhaul.
I took your patch and made a few more changes. Please let me know what you think.
div.
pieces of the selectors. I know this ends up breaking styling in Bartik, but the fact that Bartik adds padding toul
s with.block ul
is a bug in Bartik. A core module shouldn't need to make concessions for a theme-level issue.outline-offset
to the.contextual-region-active
class.Other than that, the rest of the changes look great. This will go a long way to making this piece of core more friendly to themers. I have a module that builds on contextual and it was a PITA to override some of the styling in D7.
Comment #68
mortendk CreditAttribution: mortendk commentedsomehow i coudnt add the patch :( - oh well tried to merge in + cleaned up a little bit more ;)
So i guess we need to have a discussion about how much we want in the base.css and what were gonna do with elements like contextual and others elements that is a deep part of the drupal system.
*wonder why the last patch didnt cleared* .. oh it late better get som sleep
Comment #70
jessebeach CreditAttribution: jessebeach commentedThis is the set of commands I run to create a patch. I keep a branch per issue in the 8.x repo. When I commit changes to a branch, I commit them as amendments, so the last commit to the branch is always the amalgamate of all the changes for the patch I'm working on.
git commit -m "Issue #0000000 by {author}: Description of the issue" --amend
git format-patch drupal.org/8.x --stdout > ~/path/to/patches/issue_description-0000000-0.patch
drupal.org is the name of the remote repo. Yours might be origin if you haven't changed the default.
Comment #71
mortendk CreditAttribution: mortendk commentedand a little bit more fiddeling n perfectionizing imho ;)
Comment #72
aspilicious CreditAttribution: aspilicious commentedComment #74
jessebeach CreditAttribution: jessebeach commentedI rerolled the patch with git format-patch.
There is one bit I'd like a little more discussion on:
I'm willing to bend on this given eloquent rebuttal. I don't think that a core module should be making concessions for overzealous theme styling (contextual.theme.css). I'd like to see the code above written as:
Also, we shouldn't be putting in hacks for IE7 (contextual.theme.css).
If IE7 doesn't have outlines, then I'm not too worried about that. By the time D8 ships, IE7 usage will be below 4%. Maybe even 3%. Not having an outline does not diminish the functionality of this module. The outline is simply a nice enhancement for capable browsers.
Comment #75
jessebeach CreditAttribution: jessebeach commentedSetting to needs review to rouse the bots.
Comment #76
aspilicious CreditAttribution: aspilicious commentedDon't introduce visual regressions. This means we don't need to add an IE7 hack if it wasn't available in drupal 7 either.
When writing new stuff we should add ie7 hacks until we stop supporting ie7.
Comment #77
mortendk CreditAttribution: mortendk commentedMm totally on board in removing the border ie7 hack its only to make "the powers that wanna support ie7" happy ;)
guess thats a more principal discussion though ?
but wtf lets take it here ;)
The reason for adding the 3 class .contextual-region to the .contextual-region .contextual ul.contextual-links li is to help out the people that accedential is gonna overwrite it #675608: Contextual link colors easily overwritten.
The idea was to add 3 classes then its gonna get harder to overwrite by accedent all it else would take is someting like a .node .content li a and you overwriting the contextual links :/
... so theres a meaning behind the class madness ;)
Comment #78
mortendk CreditAttribution: mortendk commentedjacine told me i should assing me self to this so i am ;)
Comment #79
mortendk CreditAttribution: mortendk commentedjacine told me i should assing me self to this so i am ;)
Comment #80
JacineUm, this is not a novice patch.
Anyway, I decided to roll a patch instead of just posting a review. This is a really hard one, so I am not 100% happy/sure, but we could go on forever with this. Here's the gist of what I did.
- Fixed some issues with RTL versions of files. Some of the styles were not properly matching up.
- Moves the display block styling for the list links and li's to the .theme.css. I expect to see bullets for this in a base.css.
- Set the right/top position to 0 in the base.css files.
- Changed the comments and the grouping for what's in the .base.css file.
- Remove the .js prefix from one of the selectors in the .base.css file. This doesn't work without JS so that's misleading.
- Did some minor code style cleanups.
- Removed the border fallback for the outline in IE7.
- Changed the selector Jesse mentioned to
.contextual .contextual-links li
- Separated the colors out for the stuff that should actually change on hover/active/focus. I can't stand when people change margins/positioning/font in those cases:
Feel free to rip on my updates, and thanks for working so hard on this one. It's definitely one of the hardest issues we've got right now.
mortendk++!
Comment #81
JacineUgh, here is the same patch without the no newlines issue.
Comment #82
JacineUgh, one more time? Sorry for the inbox spam.
Comment #83
jyve CreditAttribution: jyve commentedMy feedback on the latest patch:
- color codes are preferred as lower case, but are all uppercase here
- border-radius should be moved up to get this alphabetical:
- An extra blank line is needed here (just a detail!):
- There has been some disagreement in #29-#31 with moving the position to top /right in base.css since the module also works without that positioning. Not sure how you feel about this Jacine?
For the rest: looking good!
Comment #84
JacineYep, right. I forgot where we left off with the coding standards issue on this. :/
So... yeah. I've gone back and forth on this a gazillion times, and I'm literally torn. I think both sides of this argument have equal merit. But, if I am forced to make a decision here, I'm going to say remove the top/right properties from the .base files. I am 100% against putting values like top: 2px and right: 5px in the base file, which is why I changed them to 0/0 in the latest patch. However, this ends up meaning duplicate rules in both files, and that's not exactly ideal.
Comment #85
mortendk CreditAttribution: mortendk commentedokay another roll:
* fix the lowercase issue (yeah)
* moved the top/right outta base.css so a new that dont wanna use the theme.css file dont have to overwrite this
protecting the contextual links
I have added the .contextual-region to make sure that a theme dosnt accedentially overwrites contextual links
we dont want a situation where css like this overwrites the contextual links
.sidebar-left .block ul li a = 23
.contextual-region .contextual .contextual-links li a = 32
by addin the 3. class ( .contextual-region) it will not be as easy mistake to accedentially take over the links.
Comment #86
TR CreditAttribution: TR commentedComment #87
aspilicious CreditAttribution: aspilicious commentedWell sadly enough this needs a tiny bit of work. The font-size has to be fixed.
Morten is going to do this.
Comment #88
TR CreditAttribution: TR commentedThere's also a spelling error in there - "accedential" should be "accidental".
Comment #89
rasskull CreditAttribution: rasskull commentedBesides #87 and #88 the latest patch looks good to me
Comment #90
mortendk CreditAttribution: mortendk commentedfix the spelling error in the comment...
fixed the font sizing for bartik & cleaned it up a bit with multiple font sizings.
bartik uses a combination of em & % for the font sizing on a base of font-size: small
now its cooked down to a 80% which resemples the same size as in D7
Comment #91
aspilicious CreditAttribution: aspilicious commentedDamnit can't rtbc
Not part of this patch
Replace tabs with spaces + missing ; + 80% seems to small on my d7 vs d8 tests
8 days to next Drupal core point release.
Comment #92
mortendk CreditAttribution: mortendk commentedabout the font sizing ...sorry for the ranting but here goes on ....
I dont remember reading anywhere that the bartik theme were set in stone, or was a monolith of excellence in theme building, & we should not be afraid to change where its necessary - This is nidpicking for the sake of nidpicking- Bartik has its own set of problems and is hardly what were trying to solve here.
But to explain & show why theres a minor minor size difference between the 2 versions
Drupal7 bartik uses a 90% for the sizing of the contextual wrapper
so its contextuals 90% on bodys 76%
the bodys font-size sizing is changed in D8's bartik (dont ask me who or why)
now its 87,5% instead of the 76% in d7 (hurray)
... to make it even more fun theres a - font-size: 0.923em on ul.contextual-links in bartik - which now for real makes it fun to calculate the font-size
if we set the font-sizing to 80% this will equals to a roughly 11 px - offcourse with the subpixels rendering, which is causing this minor minor minor changes in the font sizing. I Dont think we should use our time on finding the secret sweet spot in the breaking point aroubd font-size:82.14 & font-size:82.15% (that gives the hop to 12px)
--- rant done---
To sum it up: yes the font sizing will be slightly changed for the bartik theme - and that shows one of the problems if we wanna do "pixel perfect" with em's & percentage sizings.
ooh yeah and heres a new patch changed bartik to at least use em instead of the combination of small, em & % for the contextual links - we need a principel discussion about font sizing but this is not the place to take it ;)
Comment #93
mortendk CreditAttribution: mortendk commentedgo bot go
Comment #94
aspilicious CreditAttribution: aspilicious commentedSrry it took so long for a review but I can't mark this rtbc. The differences (in stark) are to big. See screenshot. But maybe that is just me...
Btw where in this issue did we decide to use a seperate font family for this. (I think it looks cool but still want to know :) )
Ow and rtl styling doesn't get triggered somehow... (and that's more serious)
Comment #95
aspilicious CreditAttribution: aspilicious commentedOk the rtl stuff was caused by a core bug.
I'm marking this needs review again.
Why?
1) rtl works after all :)
2) everything looks good in bartk (tiny font size difference)
Why not rtbc?
1) Cntextual links have the same font everywhere now (sans-serif), like the toolbar. Is that ok?
2) Is the size of the contextual link div ok (in stark)? (not to big?)
Comment #96
aspilicious CreditAttribution: aspilicious commentedwhy do we use line-height:normal. I saw in my tests it looked better if we choose something like 0.8em.
If we play with these 3 lines we can make it like it should be.
-17 days to next Drupal core point release.
Comment #97
aspilicious CreditAttribution: aspilicious commentedThis looks better. Also fixed a small rtl issue.
Comment #98
cosmicdreams CreditAttribution: cosmicdreams commentedVery small change that riffs off of #98. This one removes the div from the div.contextual of the attach function of Drupal.behaviors.contextualLinks. Why not get rid of all the divs?
Comment #99
aspilicious CreditAttribution: aspilicious commented1) your patch doesn't contain the correct css
2) The divs are added to protect the contextual. (see previous comments in this issue).
So I still think #97 is ok
Comment #100
cosmicdreams CreditAttribution: cosmicdreams commentedagreed, #97 is the way to go.
I forgot to mention that I manually tested #97 on many different browsers (IE9, Opera, FF current, Chrome Canary, and Chrome stable), for both Stark and Bartik. Everything looked good to me.
Comment #101
cosmicdreams CreditAttribution: cosmicdreams commentedAs a result of #100, RTBC
Comment #102
webchickAssigning to jhodgdon, since this is a coding standards conformance patch.
Comment #103
jhodgdonI cannot get the patch in #97 to apply to 8.x at this time. I guess it needs a reroll, or did someone already commit it?
Comment #104
sunRegardless of docs/standards patch or not, let's give others sufficient time to review RTBC patches, please. This patch was RTBC'ed only a few hours before the commit attempt.
See also #1399824: [policy] Formalize min/max time thresholds for RTBC patches
Missing spaces in new code. That said, this entire change looks unnecessary to me.
I'm slightly concerned the generic term "contextual" as CSS class might lead to false-positives.
I don't have concrete examples at hand, but at least I'd see an increased chance for an external CSS/JS library to use exactly that class name, too.
If you don't think that's an issue, then ok.
Comment #105
webchickI just to point out there is absolutely no consensus in the referenced issue that we should add even more barriers to people trying to get their patches committed. And further, that coding standards/documentation-related patches can be much more easily fixed in follow-ups post-commit than standard bug fixes and other types of patches (for example, sun's comments here could easily be a novice task tackled during office hours), so even if there were consensus around that I would strongly advocate using a different rulestick for these types of changes.
We brought Jennifer on as a core committer to help take pressure off of catch, Dries, and I, so we could focus on majors/criticals/other bug fixes, and to help with the velocity of the RTBC queue. Makes sense to let her do her that. :)
That said, the feedback seems valid, and a quick re-roll should be able to take care of it.
Comment #106
cosmicdreams CreditAttribution: cosmicdreams commentedRerolled patch from #97
Comment #107
cosmicdreams CreditAttribution: cosmicdreams commentedComment #108
JacineContextual is the name of the module, and that is generally how we use class names for widgets modules provide. Personally, I have no concerns about clashes here TBH. Every decent jQuery plugin allows class customization, so if there ever was a clash (which I think is unlikely) it could be easily resolved.
That said, this patch needs to be rerolled to include the addition/deletion of files. See patch in #97.
Comment #109
cosmicdreams CreditAttribution: cosmicdreams commentedrerolled using git diff --staged > new_patch.patch
Comment #110
cosmicdreams CreditAttribution: cosmicdreams commentedOne more shot:
Comment #111
JacineAlright, finally we have a winner (almost)! :D
Testing-wise this is ready to go (I've browser tested this again). This last bit has to be fixed though, as @sun mentioned:
Spacing should be:
'#attributes' => array('class' => 'contextual-links'),
Comment #112
cosmicdreams CreditAttribution: cosmicdreams commentedHere ya go
Comment #113
cosmicdreams CreditAttribution: cosmicdreams commentedComment #114
JacineThanks @cosmicdreams! Looks good.
Comment #115
jhodgdon#112: 1216978-112-contextual.patch queued for re-testing.
Comment #116
jhodgdonI don't think this patch is quite ready to commit.
a) In contextual.module:
I think we still want a nested array here, so that if someone wants to page/form alter this, they can add another class. Aren't we still doing this as a general rule?
b) In bartik/css/style.css:
There needs to be a space betwee a and { in the second line.
Comment #117
jhodgdonComment #118
JacineGood catch! I've taken care of those and also 2 more things that I didn't notice before:
This font-size shouldn't be messed with. It's the wrong size and doesn't have any affect on the result anyway. All we care about here is the selector change, so, I've reverted the font-size change.
This still does target ul.contextual-links, not ul.links. Not sure when/why this was changed, but it doesn't belong, so I've reverted it as well.
Comment #119
aspilicious CreditAttribution: aspilicious commentedI think it matters in stark or in Bartik. Will recheck
Comment #120
JacineMake sure you check the computed font-size. It's coming in at 13 px regardless from what I saw. Thanks :)
Comment #121
cosmicdreams CreditAttribution: cosmicdreams commentedis there anything more to do here? manual testing?
Comment #122
Jacine@cosmicdreams Please just read the last few comments instead of asking this all the time. It doesn't help. Thanks.
Comment #123
aspilicious CreditAttribution: aspilicious commentedTested and retested. Also with RTL. R T B C it is.
Comment #124
webchickTagging with coding standards, for Jennifer to take a look at.
Comment #125
jhodgdonWe're over threshold on critical tasks at the moment, but I'll look at committing this when the numbers fall. I don't want to commit patches that remove/add files when we're over threshold, as they might interfere with other pending patches.
Comment #126
jhodgdonIt seems that none of the patches in the critical/major queue conflict with this, so I went ahead and committed it. Yeah team!
Comment #127
JacineYay, thanks @jhodgdon!! Removing the sprint tag.
Comment #129
tim.plunkettWith .contextual-links-wrapper being renamed to .contextual, and .contextual-links-region to .contextual-region, this broke Views, and would likely break any CSS out there in the wild.
I'd say this is worth a change notice. There is a checkbox for "Themers", we should use it.
If someone disagrees, we should come up with a standard for "we can change this without telling anyone".
Comment #130
xjmWe should probably create one change notification for all the CSS cleanups, and include a table of renamed classes or IDs where needed.
Comment #131
tim.plunkettApparently this is too much to ask for. In retrospect, it is *very* hard to keep track of every markup/CSS change.
Comment #132
oresh CreditAttribution: oresh commentedA new small portion of css clean up for latest build (a lot has changed during a year).
Reducing the weight of selectors.
Comment #133
xjm@oresh, please file a new issue.
Comment #133.0
xjmAdding docs.