search

freedomgames / Planet-Lab

Planet Lab: A gamified citizen science network for youth!
BSD 3-Clause "New" or "Revised" License
8 stars 16 forks source link

Login Styling #39

Closed TreeStrepek closed 9 years ago

TreeStrepek commented 9 years ago

Customized Flask login styling for PlanetLAB use.

@waltaskew

waltaskew commented 9 years ago

That favicon.ico is another thing that should live in S3 rather than the repo. Let's keep the repo just for code.

TreeStrepek commented 9 years ago

Please merge the code I just pushed up. Our PlanetLAB UI is underway and includes considerable improvements. Limited inline styling residue may be visible. Styling needs to be moved into the centralized style sheet. In cases where it has been left inline it is only because of time constraints. All styling belongs in the centralized stylesheet.

waltaskew commented 9 years ago

In cases where it has been left inline it is only because of time constraints. All styling belongs in the centralized stylesheet.

Let’s try not to start trading quality for time so early in the project, especially when the time saved by using inline css should be rather minimal.

More significantly quality-wise, this pull request breaks the CI build. The tests I had for the little proof-of-concept quest builder are failing with these changes, and it doesn't look like the tests have been updated to match the new code. We really can not be merging code when the test suite is failing. Failing tests will block the automated deployments.

screen shot 2015-04-18 at 7 28 01 am

At the very least, we can not merge code that breaks existing tests, but we should not be merging new code that does not feature new tests either.

Another blocker for merging --- all of the S3 urls point to s3-us-west-2.amazonaws.com/itreeware but they need to point to the PlanetLab S3 (well, actually they need to point to the PlanetLab CloudFront.) My offer's still open -- if you send me your public GPG encryption key (or suggest some other secure method for passing these things along) I'll send you credentials to read and write from the PlanetLab S3 bucket.

If folks want to come up with clever ideas for automatically swapping these links between dev and production S3 buckets, that sounds like something we could pull of with sed or some fancier templating thing.

waltaskew commented 9 years ago

Just as a general note -- one thousand lines is a bit long for a pull request. I find it difficult to perform effective reviews on anything longer than five hundred lines and generally shoot for around three hundred myself.

TreeStrepek commented 9 years ago

OK. Walt.  I have reviewed your comments.  Moving inline styling to centralized style sheet is no sweat.  I will take out the legal comment about an outdated license.  Regarding the blocker with our test engine failing.  Who/ when can this be corrected?  I am on this project for a limited number of hours.   How can we get this turned around in 1-2 weeks?  That's when I roll off.TheresaStrepek 

-------- Original Message -------- Subject: Re: [Planet-Lab] Login Styling (#39) From: Walt Askew notifications@github.com Date: Sat, April 18, 2015 7:47 am To: freedomgames/Planet-Lab Planet-Lab@noreply.github.com Cc: "Tree Strepek (active)" tree@iTreeware.com

In cases where it has been left inline it is only because of time constraints. All styling belongs in the centralized stylesheet. Let’s try not to start trading quality for time so early in the project, especially when the time saved by using inline css should be rather minimal. More significantly quality-wise, this pull request breaks the CI build. The tests I had for the little proof-of-concept quest builder are failing with these changes, and it doesn't look like the tests have been updated to match the new code. We really can not be merging code when the test suite is failing. Failing tests will block the automated deployments. At the very least, we can not merge code that breaks existing tests, but we should not be merging new code that does not feature new tests either. Another blocker for merging --- all of the S3 urls point to s3-us-west-2.amazonaws.com/itreeware but they need to point to the PlanetLab S3 (well, actually, they need to point to the PlanetLab CloudFront.) My offer's still open -- if you send me your public GPG encryption key (or suggest some other secure method for passing these things along) I'll send you credentials to read and write from the PlanetLab S3 bucket. If folks want to come up with clever ideas for automatically swapping these links between dev and production S3 buckets that sounds like something we could pull of with sed or some fancier templating thing. —Reply to this email directly or view it on GitHub.

TreeStrepek commented 9 years ago

My code is piling up in at the gate of the gitHub merge process.  If we move my code through the process there will be less code to review all at once.  I have heard you say the Travis CI script needs to be updated.  It is failing my code.  And it will continue to fail. It must be updated by Walt or Maia.  This is a blocker.TheresaStrepekOracle Middleware DeveloperMobile | Fusion | AngularJSiTreeware.comiTreeware.blogspot.com 

-------- Original Message -------- Subject: Re: [Planet-Lab] Login Styling (#39) From: Walt Askew notifications@github.com Date: Sat, April 18, 2015 7:51 am To: freedomgames/Planet-Lab Planet-Lab@noreply.github.com Cc: "Tree Strepek (active)" tree@iTreeware.com

Just as a general note -- one thousand lines is a bit long for a pull request. I find it difficult to perform effective reviews on anything longer than five hundred lines and generally shoot for around three hundred myself. —Reply to this email directly or view it on GitHub.

TreeStrepek commented 9 years ago

1: the itreeware S3 references are the decided  "test environment" for the app.  That decision was made by the team.  #2: The Travis CI test script needs to be updated.  Who is going to take on that task?  When can it be done?  TheresaStrepekOracle Middleware DeveloperMobile | Fusion | AngularJS 

-------- Original Message -------- Subject: Re: [Planet-Lab] Login Styling (#39) From: Walt Askew notifications@github.com Date: Sat, April 18, 2015 7:47 am To: freedomgames/Planet-Lab Planet-Lab@noreply.github.com Cc: "Tree Strepek (active)" tree@iTreeware.com

In cases where it has been left inline it is only because of time constraints. All styling belongs in the centralized stylesheet. Let’s try not to start trading quality for time so early in the project, especially when the time saved by using inline css should be rather minimal. More significantly quality-wise, this pull request breaks the CI build. The tests I had for the little proof-of-concept quest builder are failing with these changes, and it doesn't look like the tests have been updated to match the new code. We really can not be merging code when the test suite is failing. Failing tests will block the automated deployments. At the very least, we can not merge code that breaks existing tests, but we should not be merging new code that does not feature new tests either. Another blocker for merging --- all of the S3 urls point to s3-us-west-2.amazonaws.com/itreeware but they need to point to the PlanetLab S3 (well, actually, they need to point to the PlanetLab CloudFront.) My offer's still open -- if you send me your public GPG encryption key (or suggest some other secure method for passing these things along) I'll send you credentials to read and write from the PlanetLab S3 bucket. If folks want to come up with clever ideas for automatically swapping these links between dev and production S3 buckets that sounds like something we could pull of with sed or some fancier templating thing. —Reply to this email directly or view it on GitHub.

waltaskew commented 9 years ago

1: the itreeware S3 references are the decided "test environment" for the app. That decision was made by the team.

We're merging and deploying this code to production, though. It's fine to use itreeware in your development environment, but as soon as we merge this it will trigger a deployment to production, and production will be reading from itreeware which we don't want.

2: The Travis CI test script needs to be updated. Who is going to take on that task? When can it be done?

Nothing about the travis script needs to be changed. It's working super. The build is broken because you are making changes without updating the relevant tests. Check out the travis build:

  1) Quest CRUD should create and retrieve quests
   Message:
     Error: No element found using locator: by.binding("quest.name")
   Stacktrace:
     Error: No element found using locator: by.binding("quest.name")
==== async task ====
Asynchronous test function: it()
Error
    at editText (/home/travis/build/freedomgames/Planet-Lab/frontend/test/e2e/quests.js:45:44)
    at [object Object].<anonymous> (/home/travis/build/freedomgames/Planet-Lab/frontend/test/e2e/quests.js:56:13)
Error
    at [object Object].<anonymous> (/home/travis/build/freedomgames/Planet-Lab/frontend/test/e2e/quests.js:28:5)
    at Object.<anonymous> (/home/travis/build/freedomgames/Planet-Lab/frontend/test/e2e/quests.js:6:1)

The tests in frontend/test/e2e/quests.js are referring to DOM elements that no longer exist after your changes. You should update the tests so that they work with your new code, and hopefully expand them to cover the new stuff you're doing.