Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

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

LewisNyman’s picture

Issue summary: View changes
joaogarin’s picture

Reviewed 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

joaogarin’s picture

Sorry new path there was a indent problem with one of the styles in the previous one.

idebr’s picture

Status: Active » Needs review

@joaogarin Don't forget to set the issue status to 'Needs review' when you upload a patch :)

idebr’s picture

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +Needs screenshots

Thanks for the patch!

  1. --- /dev/null
    +++ b/.idea/codeStyleSettings.xml
    
    +++ b/.idea/codeStyleSettings.xml
    @@ -0,0 +1,10 @@
    
    @@ -0,0 +1,10 @@
    +<?xml version="1.0" encoding="UTF-8"?>
    +<project version="4">
    +  <component name="ProjectCodeStyleSettingsManager">
    +    <option name="PER_PROJECT_SETTINGS">
    +      <value />
    +    </option>
    +    <option name="PREFERRED_PROJECT_CODE_STYLE" value="Default (1)" />
    +  </component>
    +</project>
    +
    

    I think this file was added accidentally. Can we remove it?

  2. +++ b/core/themes/seven/css/components/buttons.css
    @@ -25,15 +25,123 @@
     /* Link actions. */
    +
    +/**
    + * Style a clickable/tappable element as a link. Duplicates the base style for
    + * the <a> tag, plus a reset for padding, borders and background.
    + */
    

    Could we move the link actions comment in with the description?

We also need some screenshots just to verify we haven't broken anything.

joaogarin’s picture

Hello,

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.

LewisNyman’s picture

Status: Needs work » Needs review
Issue tags: -Needs screenshots
idebr’s picture

Status: Needs review » Needs work
  1. The buttons.theme.css file is deleted, but it is still referenced in seven.libraries.yml
  2. /**
     * Rare instances when a dropbutton is actually just a button.
     * Copied from Seven's buttons.theme.css.
     */

    There is a reference to buttons.theme.css in dropbutton.component.css that should be updated.

  3. +++ b/core/themes/seven/css/components/buttons.css
    @@ -25,15 +25,123 @@
    +  -moz-appearance: none;  /* " */
    +  padding: 4px 1.5em;  /* 1 */
    ...
    +  font-size: 0.875rem;  /* 2 */
    ...
    +  -webkit-font-smoothing: antialiased;  /* 3 */
    

    These numbered comments should be incremented and the original comments should be added to the selector docblock.

  4. +++ b/core/themes/seven/css/components/buttons.css
    @@ -25,15 +25,123 @@
    +  color: #333333;
    

    Write shorthand color codes when possible, eg. #333 instead of #333.

joaogarin’s picture

Hello,

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.

joaogarin’s picture

Status: Needs work » Needs review
vermario’s picture

Hello!

I reviewed the patch in #13, and I have rerolled with these minor changes:

1. changed the #333333 color to the shorthand

-  color: #333333;
+  color: #333;

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

   -webkit-appearance: none;
-     -moz-appearance: none;
+  -moz-appearance: none;

3. Changed the second comment for comment number 3, having the number there instead of the / " / character (seems clearer that way, I think)

-webkit-appearance: none;  /* 3 */
-     -moz-appearance: none;  /* " */
+  -moz-appearance: none;  /* 3 */

Status: Needs review » Needs work

The last submitted patch, 13: seven-theme-mergebuttonscss-2408491-13.patch, failed testing.

joaogarin’s picture

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

vermario’s picture

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

joaogarin’s picture

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

joaogarin’s picture

Status: Needs work » Needs review

Hello, 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.

idebr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

mrjmd’s picture

Status: Needs work » Needs review
FileSize
11 KB

Here'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.

tisteegz’s picture

Issue tags: -Needs reroll
jOksanen’s picture

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

mrjmd’s picture

Looks good, however I made one more small coding standards fix, to the spacing of the wrapped line in the comment.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice
FileSize
323.58 KB

Thanks for all the work here. I manually tested the patch to make sure there were no visual regressions and it looks that same.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

CSS is not frozen in beta. Committed 262a3e7 and pushed to 8.0.x. Thanks!

  • alexpott committed 262a3e7 on 8.0.x
    Issue #2408491 by joaogarin, mrjmd, vermario, jOksanen: Rewrite button...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.