Skip to content
This repository has been archived by the owner on Jan 10, 2019. It is now read-only.

broken site button #153

Open
wants to merge 2 commits into
base: beta
Choose a base branch
from
Open

broken site button #153

wants to merge 2 commits into from

Conversation

jdorweiler
Copy link
Contributor

@jdorweiler jdorweiler commented Jul 19, 2017

Reviewer:

Description:

Added a report broken site that links to a survey.
screen shot 2017-07-19 at 12 38 52 pm

Steps to test this PR:

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@jdorweiler jdorweiler added the WIP label Jul 19, 2017
Copy link
Collaborator

@laurengarcia laurengarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments re: code. Before this is merged I want to make some comments about UX in asana and see if this can be slightly changed. Not liking the inconsistency with the right arrow usage.

@@ -1,5 +1,6 @@
const bel = require('bel');
const toggleButton = require('./shared/toggle-button');
const Button = require('./shared/button');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shared/button is a function not a constructor, so it should be const button not const Button.

$popup__height: 505px;
$popup__height--moz: 520px;
$popup__height: 540px;
$popup__height--moz: 540px;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that originally the heights across browsers were different and now they are the same. Did you verify that the height was correct in FF and Chrome? I would think that FF would be taller.

margin:0;
padding:0;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a new blank line between this and next line, and add a space between properties and values above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants