Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

702 Visually distinguish the dev site from prod #849

Merged
merged 12 commits into from
May 18, 2014

Conversation

AdmiralJota
Copy link
Contributor

Resolves #702

http://jenkins.buttonweavers.com:8080/job/buttonmen-AdmiralJota/55/

This will require that puppet overwrite Config.js on the dev site after a deploy.

Shaded the login header, changed the text on the front page, and
dynamically swapped out the favicon.
Switched site config from API call to a static javascript object.
@AdmiralJota
Copy link
Contributor Author

@cgolubi1
Copy link
Contributor

Minor comments:

  • Any reason to change the BMInterface and BMInterfaceNewuser $conn variables from private to protected? That looks like part of a change which didn't wind up happening (to add an interface base class), so i'd be inclined not to make that one-liner change either, but it's okay if you want to.
  • There's no reason you would have known this without source-diving, but the BMTestUtils.getAllElements() function which gets invoked as pre and post from each test module specifies which JS variable namespaces to query by name. So when you add a new module (Config), you have to add it to that function as well in order for the tests to actually check for unexpected variable changes during execution.

More majorly, i immediately noticed that something is wrong with the login header --- when i'm logged out, there's a lot of unnecessary vertical whitespace in the login header. The old site didn't have this problem:
login_subbar_missing

I never got around to adding real unit tests for Login.js, as you can see from the Cobertura coverage report. I don't want to ask you to fix that entire problem, but, in the interest of using unit tests to prevent regressions of problems we've seen, want to see if it's possible to make a Login.js unit test which catches the extra lines of whitespace in the header while logged out?

Otherwise, the dev favicon and grayed header look fine to me. I am not super fond of all-caps and think the gray will be obvious enough, so i would just say "ButtonMen Dev Site" in the header (i think it makes sense on the front page), but don't feel strongly.

I'll write the puppet config to change Config.js on the dev site.

@AdmiralJota
Copy link
Contributor Author

http://jenkins.buttonweavers.com:8080/job/buttonmen-AdmiralJota/88/

There's still some added whitespace in the header (both when you're logged in and when you're not), since A) the grey background needs it to not look weird, and B) I wanted to keep the header the same height whether it's dev or prod, but it's no longer egregious or aesthetically inappropriate, I don't think.

Unfortunately, I don't think that the way CSS renders whitespace is something we can test for in javascript unit tests.

I've made the other changes.

@cgolubi1
Copy link
Contributor

Thanks! I agree the whitespace looks better now. Merging.

cgolubi1 added a commit that referenced this pull request May 18, 2014
702 Visually distinguish the dev site from prod
@cgolubi1 cgolubi1 merged commit 8410a99 into buttonmen-dev:master May 18, 2014
@AdmiralJota AdmiralJota deleted the 702_distinguish_dev_site branch May 18, 2014 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make the dev site look more visually different from the prod site
2 participants