Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
See: #2405553: Ensure Seven's code is inline with the current standards
Remaining tasks
Review the current CSS — What to look for when reviewing CSS
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#25 | Screen Shot 2015-03-08 at 11.42.20.jpg | 323.58 KB | LewisNyman |
#24 | seven-theme-mergebuttonscss-2408491-24.patch | 11.47 KB | mrjmd |
#13 | interdiff-2408491-11-13.txt | 957 bytes | vermario |
#13 | seven-theme-mergebuttonscss-2408491-13.patch | 10.99 KB | vermario |
#11 | Screenshot 2015-02-06 12.09.47(2).png | 1.74 MB | joaogarin |
Comments
Comment #1
LewisNyman CreditAttribution: LewisNyman commentedThe first thing we need to do is merge button.theme.css into buttons.css. I don' t think there's any benefit in splitting the properties in themes the same way we do in modules. After that is done we can see if we can improve the rest of the code.
Comment #2
LewisNyman CreditAttribution: LewisNyman commentedComment #3
joaogarin CreditAttribution: joaogarin commentedReviewed the code on both css files. Looked okay.
I merged the css styles.
button--danger looks a bit misplaced after the link style. Maybe should be moved to next to the modifier "primary" ?
Best regards
Comment #4
joaogarin CreditAttribution: joaogarin commentedSorry new path there was a indent problem with one of the styles in the previous one.
Comment #5
idebr CreditAttribution: idebr commented@joaogarin Don't forget to set the issue status to 'Needs review' when you upload a patch :)
Comment #6
idebr CreditAttribution: idebr commentedComment #7
LewisNyman CreditAttribution: LewisNyman commentedThanks for the patch!
I think this file was added accidentally. Can we remove it?
Could we move the link actions comment in with the description?
We also need some screenshots just to verify we haven't broken anything.
Comment #8
joaogarin CreditAttribution: joaogarin commentedHello,
Here goes a new patch. removed the unwanted file and moved the action links comment in with the rest of the description.
Also attaching some print screens of the buttons.
Comment #9
LewisNyman CreditAttribution: LewisNyman commentedComment #10
idebr CreditAttribution: idebr commentedThere is a reference to buttons.theme.css in dropbutton.component.css that should be updated.
These numbered comments should be incremented and the original comments should be added to the selector docblock.
Write shorthand color codes when possible, eg. #333 instead of #333.
Comment #11
joaogarin CreditAttribution: joaogarin commentedHello,
Did the revisions by @idebr :
Removed reference from seven.libraries.yml
Changed reference in dropbutton.component.css (now referencing buttons.css)
Reworked the comments in the buttons.css stylesheet
I am adding also some screenshots.
Comment #12
joaogarin CreditAttribution: joaogarin commentedComment #13
vermario CreditAttribution: vermario commentedHello!
I reviewed the patch in #13, and I have rerolled with these minor changes:
1. changed the #333333 color to the shorthand
2. Removed the indentation for the -moz- vendor properties, because I could not find indication that we want this indentation in the css guidelines (correct me if I'm wrong!) :)
3. Changed the second comment for comment number 3, having the number there instead of the / " / character (seems clearer that way, I think)
Comment #16
joaogarin CreditAttribution: joaogarin commentedI don't think there are any references regarding the comments or the indentation so I decided to leave them like they where.
I actually think the comment /* '' */ makes sense as this is a universal symbol for referencing what is on top, so in that sense the indentation would make sense because if references what is exactly above
http://en.wikipedia.org/wiki/Ditto_mark
Anyway as there is no reference in the guides I think we can agree on a good solution and go with it;)
Comment #17
vermario CreditAttribution: vermario commentedOk, I understand the reason now!
My opinion on this would be to go with removing the indentation however, because of what this says:
https://www.drupal.org/node/1887862#whitespace
We need a third opinion probably :)
Comment #18
joaogarin CreditAttribution: joaogarin commentedYes looking at https://www.drupal.org/node/1887862#blank-lines it actually has the indentation on :
* A comment describing the ruleset. */
.selector-1,
.selector-2,
.selector-3[type="text"] {
-webkit-box-sizing: border-box;
-moz-box-sizing: border-box;
Altough its talking about the blank lines and not indentation I think we can see from the example that is using it. Anyway yes lets have someone else give their opinions;)
Comment #19
joaogarin CreditAttribution: joaogarin commentedHello, how do we proceed with this? Proceed from #11 or #13?
i can incorporate the change for the minified color #333 into #11 and put the new patch.
Comment #20
idebr CreditAttribution: idebr commentedThe patch no longer applies:
error: patch failed: core/themes/seven/seven.libraries.yml:12
error: core/themes/seven/seven.libraries.yml: patch does not apply
Regarding the white-space: there is no explicit rule regarding the use of white-space in vendor prefixes that I am aware of, but currently in HEAD the version without additional white-space is more common so let's stick with that for consistency.
Comment #21
mrjmd CreditAttribution: mrjmd commentedHere's a straight reroll of #13.
I'm going to agree with @vermario here that using the number instead of the " ensures no one will be confused, and removing the indentation looks better, although I don't think either change is necessarily dictated by the coding standard.
Comment #22
tisteegz CreditAttribution: tisteegz commentedComment #23
jOksanen CreditAttribution: jOksanen commentedWent through the code standards. No indents anywhere for vendor prefixes. Found two more in the file and made patch to work with those as well. Applies well to latest Dev.
Interdiff in a weird format but you can see the changes easily still. The diff is very minimal from the last one, and could mark as RTBC really quickly.
Comment #24
mrjmd CreditAttribution: mrjmd commentedLooks good, however I made one more small coding standards fix, to the spacing of the wrapped line in the comment.
Comment #25
LewisNyman CreditAttribution: LewisNyman commentedThanks for all the work here. I manually tested the patch to make sure there were no visual regressions and it looks that same.
Comment #26
alexpottCSS is not frozen in beta. Committed 262a3e7 and pushed to 8.0.x. Thanks!