Skip to content
This repository has been archived by the owner on Mar 15, 2018. It is now read-only.

move buttons (bug 1161854) #1195

Merged
merged 1 commit into from May 13, 2015
Merged

move buttons (bug 1161854) #1195

merged 1 commit into from May 13, 2015

Conversation

ngokevin
Copy link
Contributor

@ngokevin ngokevin commented May 8, 2015

  • Make more use of Stylus parent selectors (don't know what they're called) for component-izey CSS.
  • Add some more namespaced class names.
  • Fix invalid HTML in our app-tiles (trying to put block elements in an <a> tag)
  • Move logic from button template
    screen shot 2015-05-12 at 3 10 30 pm
    screen shot 2015-05-12 at 4 14 19 pm
    screen shot 2015-05-12 at 4 14 15 pm
    screen shot 2015-05-12 at 3 10 23 pm
    screen shot 2015-05-12 at 4 14 10 pm
    screen shot 2015-05-12 at 4 15 45 pm
    screen shot 2015-05-12 at 3 09 46 pm

@ngokevin ngokevin force-pushed the buttonmove branch 2 times, most recently from 21d0bc2 to 3e5fb43 Compare May 11, 2015 21:10
@ngokevin
Copy link
Contributor Author

@pwalm quick look?

@mstriemer r?

@ngokevin ngokevin changed the title [WIP] move buttons (bug 1161854) move buttons (bug 1161854) May 11, 2015
@pwalm
Copy link

pwalm commented May 12, 2015

Looks good, but I think we're going to move towards keeping everything in the button in phase one though. The separate word outside the button didn't hold up very well in a design critique.

An idea of where we might go:
screen shot 2015-05-12 at 10 17 47 am

@ngokevin
Copy link
Contributor Author

Should I just make the buttons look like that now?

@ddurst
Copy link
Contributor

ddurst commented May 12, 2015

(Yeah, agree on that question.)

@pwalm
Copy link

pwalm commented May 12, 2015

Yeah, might as well! Thanks Kevin.

/* Button Shape: /
background: #0092FF;
border-radius: 5px;
/
Install for [price]: */
font-family: FiraSansOT;
font-size: 12px;
color: #FFFFFF;
line-height: 16px;

@chuckharmston
Copy link
Contributor

I don't think we want line-height equal to anything but the text size here, that will make the text look a bit vertically off-centered within the button unless we unevenly apply padding.

@pwalm
Copy link

pwalm commented May 12, 2015

👍

@ngokevin
Copy link
Contributor Author

Is the button still stacked?

@pwalm
Copy link

pwalm commented May 12, 2015

As in stacked under name/dev/rating? Yep.

@@ -50,7 +42,7 @@ $btn-install-font-size = 12px;
}
}
&.installed,
&.purchased,
purchased,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right?

@ngokevin ngokevin force-pushed the buttonmove branch 2 times, most recently from 4a42b80 to 3ccd7dc Compare May 12, 2015 23:14
@ngokevin
Copy link
Contributor Author

updated

@mstriemer
Copy link
Contributor

This buttons look great, good work guys!

data-product="{{ app|json }}">
{% macro market_button(app, data_attrs) %}
{% set app = buttons.transformApp(app) %}
<span class="button mkt-app-button product install
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change to <span> from <button>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a reason...let me find out.

@mstriemer
Copy link
Contributor

r+wc

The button becoming a span seems weird but looks good.

@ngokevin ngokevin force-pushed the buttonmove branch 4 times, most recently from 4f96a1a to 0f74297 Compare May 13, 2015 22:36
ngokevin added a commit that referenced this pull request May 13, 2015
@ngokevin ngokevin merged commit ead91ee into mozilla:master May 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants