1218783 - [RedSquare] gaia_grid should be copied since some changes in this shared compoent are needed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Description

[Alexey Yakimov]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.71 Safari/537.36

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] MariaKolesina:bug-1218783 > mozilla-b2g:v2.2r

Comment 2

[Alexey Yakimov]

Remove needinfo flag set by mistake.

Comment 3

[Alexey Yakimov]

Comment on attachment 8679471 [details] [review]
[gaia] MariaKolesina:bug-1218783 > mozilla-b2g:v2.2r

This is just a pure copy of gaia_grid component which is needed for redsquare project.

I don't see any option to set "review" flag for just uploaded patch, so it could be some unnecessary actions for this bug from my side and I want to excuse for it in advance.

Comment 4

[Alexey Yakimov]

David, please proceed with formal review and landing of this code. This is a pure copy of gaia_grid.

Comment 5

[David Flanagan [:djf]]

Landed in v2.2r: https://github.com/mozilla-b2g/gaia/commit/d31d028332fd68eb80e50e917b2c06e62a771297

Comment 6

[Carsten Book [:Tomcat]]

this caused a perma failure like https://treeherder.mozilla.org/logviewer.html#?job_id=12091&repo=mozilla-b2g37_v2_2r now on 2.2r 

David can you fix this or do we need to revert his ?

Comment 7

[Carsten Book [:Tomcat]]

sorry seems this was perma failure and so i revered this now in https://github.com/mozilla-b2g/gaia/commit/4e950c10285dcc6ace9342e584bcb81581a4b453 - also i guess this would have needed approval before merged to 2.2r

Comment 8

[David Flanagan [:djf]]

Carsten,

I'm the tech lead for the Red Square project that is using the 2.2r branch, and am one of the only people who will be landing stuff there, so I was self-approving.  I didn't realize it got any attention from sheriffs.

The tests were not triggered automatically when the PR was submitted... Can you help me understand how to get the tests to run before landing things like this?  If it doesn't happen automatically, I'm kind of helpless.

Comment 9

[David Flanagan [:djf]]

Alexey,

The test that failed here was a CSS lint test. The original gaia-grid component has CSS linter failures that are accounted for in build/csslint/xfail.list with this line:

  shared/elements/gaia_grid/style.css 0 5

As part of copying the element to gaia_grid_rs you need to also copy the line above and add _rs in the copy.

All the linting steps should run automatically when you attempt to commit changes to your gaia repo, so in theory lint failures should never turn up in the automated tests. In order to enable that, you need to have node installed, and then run 'npm install' in your gaia directly. This will install a bunch of stuff including a commit hook that will run the linters before allowing a patch to be committed.

Please make the change above and resubmit the patch in a new PR.

On my end, I need to figure out how to get the automated tests to run before I land your patches so that we don't have this kind of back out in the future.

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] MariaKolesina:bug-1218783 > mozilla-b2g:v2.2r

Comment 11

[Alexey Yakimov]

David, thanks for pointing this out. 

We should have checked the CSS style before commiting the patch to Bugzilla. We've been introducing this practice on our side now, so all subsequent PRs will be verified by CSS lint on our side before submitting them for review. There could be failures in already submitted PRs though, but we intend to correct them before submitting the next patch to Mozilla (in addition to providing the fixes for your comments).

In regards to this particular bug, we have added gaia_grid_rs into xfail.list, so now the CSS lint test should be passed.

Comment 12

[Carsten Book [:Tomcat]]

landed as https://github.com/mozilla-b2g/gaia/commit/8ea494e3c6a0198dbfa3477939961a34e1bcac09