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

fix: Layout of themes and extensions home #2100

Merged
merged 22 commits into from Mar 24, 2017

Conversation

Projects
None yet
3 participants
@mstriemer
Copy link
Member

commented Mar 19, 2017

This is #2024 but rebased with master to remove the extra commits and fix a test failure on master.

cc @aayushsanghavi

screen shot 2017-03-22 at 11 37 21 screen shot 2017-03-22 at 11 37 31

@aayushsanghavi

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2017

@mstriemer thank you so much for this. So with this up, I guess we should close #2024 and ask someone to review this PR. Also, the changes you've made in your last commit look good, thanks again.

@mstriemer mstriemer requested a review from tofumatt Mar 22, 2017

@tofumatt
Copy link
Member

left a comment

Needs a few changes but largely looks good. gettext stuff needs fixing and there are things I think could use variables, etc.

Let me know when you need another review.

@@ -108,9 +112,43 @@ export class LandingPageBase extends React.Component {
}

const { addonType, html } = content;
const themeText = i18n.gettext(
oneLine`Change your browser's appearance. Choose from thousands of themes to give Firefox the

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

We no longer use templates string tags in i18n.gettext, so we can remove oneLine here and anywhere else.

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 22, 2017

Author Member

It looks like we don't raise lint errors for lines that have strings in them over 100 columns? I don't get how that works but I guess that makes this easier.

@@ -20,6 +21,8 @@ import {
visibleAddonType as getVisibleAddonType,
} from 'core/utils';
import translate from 'core/i18n/translate';
import Icon from 'ui/components/Icon/index';
import Button from 'ui/components/Button/index';

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

Very pedantic here, but we could alphabetise these 😄


return (
<div className={classNames('LandingPage', `LandingPage-${addonType}`)}>

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

This newline doesn't seem needed.

<Icon name={visibleAddonType} />
<div className="LandingPage-header-text">
<h1 className="LandingPage-heading">
{addonType === ADDON_TYPE_THEME

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

This means anything not a theme gets output as an extension but that's not 100% true. If this were a dictionary or open search plugin it would say extension. I guess that's kinda how we treat them, but I'd rather see a mapping for this similar to what we do elsewhere so it's "Theme"/"Extension"/"Add-on" if unknown.

}

.LandingPage-header-text {
@include margin-start(20px);

This comment has been minimized.

Copy link
@tofumatt
@include margin-start(20px);

display: inline-block;
max-width: 350px;

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

How does this look on tablets/desktop/larger screens? Where does this number come from?

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 22, 2017

Author Member

Here it is at 800x600. I just made up the number for something that seemed like a reasonable width to wrap at.

screen shot 2017-03-22 at 18 26 35

}

.LandingPage-heading {
font-size: 14px;

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

These should be using variables (preferably the font size variables we already have).

font-variant: all-small-caps;
justify-content: center;
margin: 0 $padding-page;
padding: 1em;

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

We don't use em most of the time, so this seems out-of-place. I'd rather stick to the same units everywhere, px here would be better I think.

background: url('./themes.svg') center no-repeat;
background-size: cover;
height: 50px;
min-width: 41.5px;

This comment has been minimized.

Copy link
@tofumatt

tofumatt Mar 22, 2017

Member

These values (and the ones below) seem very strange. Should the SVG have better sizes or defaults?

This comment has been minimized.

Copy link
@aayushsanghavi

aayushsanghavi Mar 22, 2017

Contributor

@tofumatt I had adjusted these values to preserve the aspect ratio of the original icons. Also, I fixed one parameter to 50px (49.5px I guess for the extensions icon) and adjusted the other parameter accordingly because 50px seemed about right after being rendered on the screen.

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 22, 2017

Author Member

The extensions icon is 139x134 and the themes icon is 112x135. I think generally the values we get from the mocks are 3x bigger than what we want and those are pretty closed to what these are so I just divided by 3 in each dimension. The sizes are slightly smaller now.

@aayushsanghavi

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2017

Thanks for the review @tofumatt!

@mstriemer

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2017

I think all the comments have been addressed.

@tofumatt

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

Awesome, yes, sorry I've just been at other stuff during the work week in London. r+, I'll squash and merge if everyone's cool with that.

@tofumatt tofumatt merged commit cbc8402 into mozilla:master Mar 24, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@aayushsanghavi

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2017

@tofumatt thanks a lot for merging it!

@mstriemer mstriemer deleted the mstriemer:aayushsanghavi-layout_themes_home branch Mar 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.