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

Unjankify a few things #359

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Unjankify a few things #359

wants to merge 5 commits into from

Conversation

alanlgirard
Copy link
Collaborator

@alanlgirard alanlgirard commented Oct 6, 2016

Home page idea card updates

  • IE text overlap
  • Screen responsiveness
    • wraps properly on xs
    • card fills height (Note: We will need a filler image)
    • Proper padding in-between cards on ideaBrowse

Fixed #327 to fill height with card

@davethenipper: Pull this down and check IE

Copy link
Member

@YashdalfTheGray YashdalfTheGray left a comment

Choose a reason for hiding this comment

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

The code looks good; can I get a picture of the visual changes that happened as a result of this? We don't really need before/after pictures but something that shows what changed would be good.

@davethenipper
Copy link
Collaborator

davethenipper commented Oct 6, 2016

From my testing:
Both IE and Chrome: There is a lot of white space (marked in red) between the title and description. How much do we want?
image

@davethenipper
Copy link
Collaborator

davethenipper commented Oct 6, 2016

IE only: The learn page displays only a light bulb and requires scrolling down. Chrome has text on the bottom of the screen.
image

@davethenipper
Copy link
Collaborator

davethenipper commented Oct 6, 2016

IE only: Issue #327 still exists. This can be seen on the create idea page, it appears to be lower than before.

(Failed attempt at highlighting it below)
image

@davethenipper
Copy link
Collaborator

IE only: Logo appear to be lower quality
image

@davethenipper
Copy link
Collaborator

Both IE and Chrome: The login prompt has more white space in between username and password than before.

image

@davethenipper
Copy link
Collaborator

These are all pretty nit picky. Choose what you want to fix.

No major issues. Done testing!

@YashdalfTheGray
Copy link
Member

The logo issue is a non-starter. I'm not going to add image optimization for a shittier browser. We display a banner that says "Use a better browser".

@YashdalfTheGray
Copy link
Member

YashdalfTheGray commented Oct 6, 2016

The login spacing is also a non-starter. It doesn't look bad anymore. The bigger issue is that the button isn't in the right place in IE. But whatever, again, that browser sucks deer dick. If anything, we should make the banner to use a non shitty browser even larger and bright yellow.

@YashdalfTheGray
Copy link
Member

#327 is probably also a non-starter. Make that "use a better browser" banner larger and call it a day.

@YashdalfTheGray
Copy link
Member

@alanlgirard it's probably worth looking into the extra spacing on the card after the title.

@YashdalfTheGray
Copy link
Member

@alanlgirard learn looks okay on chrome, evidently not well on IE. I care more about this than the other IE issues since it blocks the main purpose of the page but I care about this way less than the extra spacing on the idea card which is also minor. It's up to you if you want to fix it or not.

@davethenipper
Copy link
Collaborator

Approved by me!

@YashdalfTheGray
Copy link
Member

@alanlgirard status?

@davethenipper
Copy link
Collaborator

@alanlgirard
This has 3 points left of improvement?

  1. Larger don't use IE banner
  2. Reduce spacing on card title
  3. IE Learn page fix

Correct?

@YashdalfTheGray We really need to get this merged and up and running on the machine under my desk before you leave. Pending Alan's response, what are your thoughts on merging this, making a 1.1 label and getting it up so we can test it? Phrasing.

@YashdalfTheGray
Copy link
Member

I'm okay with it once the conflicts are solved?

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.

3 participants