The CSS variables are declared in a JS file used by postcss. It cause issues when sending patches because linting catches undeclared variables and fails patches. This is also a discovery issue since people don't check js files for css variables.

Patch moves variables to a css file so that linting works, IDE can suggest them, and make variables more easily discoverable.

CommentFileSizeAuthor
#7 claro-variables-3027004-7.patch5.15 KBnod_
claro-variables.patch5.08 KBnod_

Comments

nod_ created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, claro-variables.patch, failed testing. View results

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Could you please keep the original comments and comment styles?

nod_’s picture

nod_
Primary language French
Location Lille
commented

I didn't expect the patch to fail so hard :'(

In any case, comment style is not possible to keep, // is not a CSS comment, only /* */ works in css files

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

@nod_

Convert block-like comments 

//
// Comment.
//

to 

/**
 * Comment.
 */

convert one-line comments
// Comment.
to
/* Comment. */

and keep line-end comments, e.g.
--var-declaration: "somevalue", // Comment
should become
--var-declaration: "somevalue", /* Comment */

nod_’s picture

nod_
Primary language French
Location Lille
commented

sorry about that huzooka, misunderstood the comment. I'll get that fixed. We'll still have the underlying issue of those issues: 

Expected RBRACE at line 2, col 3.

though.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new claro-variables-3027004-7.patch5.15 KB

Fixed comments.

Moved the file to base/variables.css makes more sense to me. gave up for csslint, it doesn't support custom vars, would be helpful to turn this off completely.

Status: Needs review » Needs work

The last submitted patch, 7: claro-variables-3027004-7.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

I like this.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Let me merge this in two commits to keep the history of our variables file...

  • huzooka committed 6e6e814 on 8.x-1.x authored by nod_ 
    Issue #3027004 by nod_: Fix variables declarations
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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