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 #2024

Closed
wants to merge 32 commits into from

Conversation

Projects
None yet
4 participants
@aayushsanghavi
Copy link
Contributor

commented Mar 8, 2017

Fixes: #1744 #1745
There is some merge conflict which would need to be fixed.

After -
screenshot from 2017-03-09 00-30-05
screenshot from 2017-03-09 00-30-15

@aayushsanghavi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

This PR somehow contains commits from my previous PR #1848.

@@ -108,9 +109,29 @@ export class LandingPageBase extends React.Component {
}

const { addonType, html } = content;
const themeText = 'Change your browser’s appearance. Choose from thousands of themes to give Firefox the look you want.';

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

These will need to be wrapped in i18n.gettext(). You can get i18n from props by adding it to line 96.


<button className="Browse-Button">
<Icon name="browse" />
<span className="Browse-by-Category-Text">browse by category</span>

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

This will need to be wrapped in i18n.gettext(), too. Can you format it as "Browse by category" as well, please?

@mstriemer
Copy link
Member

left a comment

A few more changes here. Mostly moving things around and changing class names. I'd like the button component that is used on the homepage and here to be shared, hopefully using Link gets us most of the way there and just the colour styles can be moved in to make that work.

Thanks for doing this!


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

<div className="Addon-Header">

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

Our class names are formatted as ComponentName-element-name[--state]. So this should be something like LandingPage-header. The other classes will need to be updated as well.

You shouldn't need --state in any of these but that would be for something like install switches where it would have a normal class like InstallSwitch and then when it is in the installing state (flipped on with animated stripes) you add InstallSwitch--installing then once it is installed you remove that and add InstallSwitch--installed.

<div className="Addon-Header">
<Icon name={classNames(`${addonType}`)} className="Addon-Icon" />
<div className="Addon-Header-Text">
<span className="Addon-Header-Text-Bold">

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

Try not to describe the look of the element but instead what element it is and add the look in the CSS. So this should be something like LandingPage-heading and then the fact that it is bold is stored in the CSS.

This could probably be an h1 too.

<Icon name={classNames(`${addonType}`)} className="Addon-Icon" />
<div className="Addon-Header-Text">
<span className="Addon-Header-Text-Bold">
{addonType === 'persona' ? 'Themes' : 'Extensions' }

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

These strings should be translated.

This comment has been minimized.

Copy link
@aayushsanghavi

aayushsanghavi Mar 8, 2017

Author Contributor

How do I translate them?

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

Using i18n.gettext(), they need to be wrapped individually.

// There's also an extra space before the closing } that should be removed
{addonType === 'persona' ? i18n.gettext('Themes') : i18n.gettext('Extensions')}
</div>
</div>

<button className="Browse-Button">

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

This should link to the category page for this extension type. We have a Link component in amo/components/Link and this should use that. There are styles for the white/blue colour scheme in Home.scss.

Ideally the common styles would move into Link and this colour scheme can be chosen with something like <Link appearance="light">...</Link>. Since Link uses the @include button() mixin you should be able to use the colour from the homepage link and set a class like Link--light that will use the mixin as @include button(#fff, $link-color).

This comment has been minimized.

Copy link
@aayushsanghavi

aayushsanghavi Mar 10, 2017

Author Contributor

@mstriemer can you please share the links of the categories page for each of the addon types?

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

The extension categories should work with <Link href="/extensions/categories/" /> and themes are at <Link href="/themes/categories/" />. For some reason the themes view doesn't return anything on stage.

@tofumatt do you know why there are no theme categories at https://addons.allizom.org/en-US/android/themes/categories/?

text-align: center;
}

.Icon-persona {

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

These icon styles can go into the Icon component's styles. The icon files themselves can be moved next to the component as well.

color: blue;
height: 55px;
font-size: 12px;
margin-left: calc(50% - 150px);

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

It would be nice to merge this style with the homepage's buttons. The styles there use calc as well but I think they could just be margin: 0 20px instead.

You should be able to remove the border-radius, font-size and width styles.


.Addon-Header-Text {
display: inline-block;
margin-left: 20px;

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

We need to support RTL as well so anything like margin-left, padding-left or left will need to be flipped for RTL.

There are some helpers you can use in mixins.scss. This could instead be @include margin-start(20px).


.Browse-by-Category-Text {
bottom: 10px;
position: relative;

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 8, 2017

Member

These styles seem a little odd to me. Using position: relative and position: absolute should generally be avoided in my opinion. If you need to move it up or down a bit can you use margin instead? Hopefully this shouldn't be needed though. Setting the line-height to match the container height can work to vertically centre but that can get wonky if the text wraps in a more verbose language than English.

Maybe take a look at how the buttons on the homepage work for this.

This comment has been minimized.

Copy link
@aayushsanghavi

aayushsanghavi Mar 8, 2017

Author Contributor

I tried it with margin, padding and line-height but that didn't work. Then I tried position which seemed to work somehow.
I'll have a look at the home page button styling and try to replicate that here too.

@mstriemer

This comment has been minimized.

Copy link
Member

commented Mar 8, 2017

If you want to clear out those old commits I find the best way is to start a new branch off of master and use git cherry-pick to pull in the commits you want from oldest to most recent. If it looks good you can delete this branch and recreate it from the new one you made.

@aayushsanghavi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

@mstriemer thanks a lot for the review. I'll get back to you with all the changes soon.
Also, I pulled the upstream master branch and I'm having a merge conflict there too so I'll have to fix that first. I didn't know about git cherry-pick. You taught me something new today! :)

@aayushsanghavi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

@mstriemer I've read and understood your comments but I guess while making the changes I'll require your help. I'll ping you on IRC in that case, if that's fine with you.


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

<div className="LandingPage-Header">

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

Only the component name should be capitalised for the classes, LandingPage-header.


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

<div className="LandingPage-Header">
<Icon name={classNames(`${addonType}`)} />

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

Just <Icon name={addonType} /> should work here.

{addonType === 'persona' ? i18n.gettext('Themes') : i18n.gettext('Extensions')}
</h1>
<p className="LandingPage-Heading-Content">
{addonType === 'persona' ? i18n.gettext(themeText) : i18n.gettext(extensionsText)}

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

These are already translated and don't need the extra i18n.gettext() call.

margin-top: 5px;
}

Link--light {

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

This is missing the . before Link--light. This should be moved into the styles for Link too.

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

This should move to Button actually, it will call Link and provide the button styles.

background: url('./bullet-list.svg') 50% 50% no-repeat;
background-size: cover;
height: 30px;
margin-right: 1em;

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

This should use @include margin-end(1em). The margin styles for these icons should probably move into where they're being used, too. You can add className="LandingPage-browse-icon" to the Icon call and then move this to a rule in LandingPage.scss, same with the other two.

</div>
</div>

<Link className="Browse-Button" appearance="light"

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

This should actually use Button from 'ui/components/Button'. You'll need to add support for the light appearance too. Something like:

diff --git a/src/ui/components/Button/index.js b/src/ui/components/Button/index.js                                                                                                                                                            
index 6b9e0160..b02c89cc 100644                                                                                                                                                                                                               
--- a/src/ui/components/Button/index.js                                                                                                                                                                                                       
+++ b/src/ui/components/Button/index.js                                                                                                                                                                                                       
@@ -6,6 +6,7 @@ import './Button.scss';                                                                                                                                                                                                       
                                                                                                                                                                                                                                              
 export default class Button extends React.Component {                                                                                                                                                                                        
   static propTypes = {                                                                                                                                                                                                                       
+    appearance: PropTypes.string,                                                                                                                                                                                                            
     children: PropTypes.node,                                                                                                                                                                                                                
     className: PropTypes.string,                                                                                                                                                                                                             
     href: PropTypes.string,                                                                                                                                                                                                                  
@@ -13,14 +14,16 @@ export default class Button extends React.Component {                                                                                                                                                                     
   }                                                                                                                                                                                                                                          
                                                                                                                                                                                                                                              
   static defaultProps = {                                                                                                                                                                                                                    
+    appearance: undefined,                                                                                                                                                                                                                   
     size: 'normal',                                                                                                                                                                                                                          
   };                                                                                                                                                                                                                                         
                                                                                                                                                                                                                                              
   render() {                                                                                                                                                                                                                                 
-    const { children, className, href, size, ...rest } = this.props;                                                                                                                                                                         
+    const { appearance, children, className, href, size, ...rest } = this.props;                                                                                                                                                             
     const props = {                                                                                                                                                                                                                          
       className: classNames('Button', className, {                                                                                                                                                                                           
         'Button--small': size === 'small',                                                                                                                                                                                                   
+        'Button--light': appearance === 'light',                                                                                                                                                                                             
       }),                                                                                                                                                                                                                                    
       ...rest,                                                                                                                                                                                                                               
     };                                                                                                                                                                                                                                       
.LandingPage-Header {
margin: 10px;
max-width: 100%;
text-align: center;

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

I think you can remove this and the other text-aligns from this file.


.Browse-Button {
height: 55px;
margin-left: calc(50% - 150px);

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

I think you can do: margin: 0 10px; width: 100% instead of this.

background-size: cover;
display: inline-block;
height: 50px;
margin-bottom: 15px;

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 10, 2017

Member

I don't think this bottom margin will work properly. It looks about right in English but the size of the container beside it can change based on the language. You can instead use flexbox to vertically centre it.

You'll want to set it up so you've got two containers that you can align within the parent, and fortunately it's already setup like that.

Then in CSS you can set the parent to display: flex and and tell the icon to be centred vertically.

.LandingPage-header {
  display: flex;
}

.LandingPage-icon {
  align-self: center;
}

There's a good guide to flex box on CSS Tricks that I always end up referencing since there are so many options.

This comment has been minimized.

Copy link
@aayushsanghavi

aayushsanghavi Mar 10, 2017

Author Contributor

Hahaha yeah when I was working on Mozilla's Network Pulse, my mentor mmavis shared the same link. It is a very nicely written blog.

@aayushsanghavi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2017

@mstriemer thanks for your review and sorry for the late response.
I've made the changes as you suggested but for some reason in the button component I couldn't work with the --light appearance class. I couldn't override the style rules of @include button(); in the initial lines of Button.scss. Please help with that. For now I've specified my own background-color and color rules in the Browse-button class.

@mstriemer
Copy link
Member

left a comment

Sorry for the late review.

I went through and poked around with this locally. I think getting the buttons sharing CSS will take some extra work so that can happen later. I've got a partial patch for it now so I can look at getting that done when this lands.

We'll need to make some more changes to Button to get it to link properly unfortunately. It's using a regular react-router Link and we want it to use the amo Link (this isn't totally right but will work for now, see #2053). It's also using href but should be using to instead, this should only affect one other call site, sorry this is extra work though 😞.

So in Button/index.js (and related tests which should fail after the change) change all references of href to to. In the imports it should be import { Link } from 'amo/components/Link instead of the import from react-router.

The other call site is in InstallButton/index.js, you'll need to change line 47 to have <Button to={addon.installURL} instead of using href.

You've got some conflicts that are preventing the tests from running and I think the linter is going to pop up some errors.

Thanks for updating this, sorry it's growing a bit. If I wasn't clear or you have any other questions just let me know. Happy to discuss over IRC if you want to talk through some stuff.

Do you mind updating the screenshots when you update this? It's looks really good! 😄

const props = {
className: classNames('Button', className, {
'Button--small': size === 'small',
'Button--light' : appearance === 'appearance',

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

This should be appearance === 'light'.

This comment has been minimized.

Copy link
@aayushsanghavi

aayushsanghavi Mar 16, 2017

Author Contributor

@mstriemer so sorry for this, you had to review such mistakes. I feel like laughing at myself.

children: PropTypes.node,
className: PropTypes.string,
href: PropTypes.string,
size: PropTypes.string.isRequired,
}

static defaultProps = {
appearance: 'undefined',

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

This should be undefined instead of the string 'undefined'.

position: relative;
}

.LandingPage-browse-icon {

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

These icon styles should move into Icon/styles.scss so you end up with something like:

// Icon/styles.scss
.Icon-browse {
  // The other backgrounds in this file all use `center no-repeat` so this should match.
  background: url('~ui/components/Icon/bullet-list.svg') center no-repeat;
  background-size: cover;
  // I think 20px is a better size.
  height: 20px;
  width: 20px;
}

// LandingPage.scss
.LandingPage-browse-icon {
  @include margin-end(1em);
}
.LandingPage-persona-icon {
background: url('~ui/components/Icon/themes.svg') 50% 50% no-repeat;
background-size: cover;
display: inline-block;

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

These styles should move to Icon/styles.scss too, but you shouldn't need display: inline-block or margin-bottom: 15px.

The selectors should be .Icon-themes and .Icon-extensions.

width: 90%;
}

.Browse-button-text {

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

These styles can be removed along with its corresponding <span>.


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

<div className="LandingPage-header">
<Icon name={addonType} className={classNames(`LandingPage-${addonType}-icon`)}/>

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

Can you use visibleAddonType instead of addonType here? It will be extensions or themes instead of extension or persona.

<Icon name={addonType} className={classNames(`LandingPage-${addonType}-icon`)}/>
<div className="LandingPage-header-text">
<h1 className="LandingPage-heading">
{addonType === 'persona' ? i18n.gettext('Themes') : i18n.gettext('Extensions')}

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

You can use ADDON_TYPE_THEME instead of 'persona' here.

}

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

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

This text is a little big. Can you use font-size: 14px; margin: 0 instead, please?

}

.LandingPage-heading-content {
margin-top: 5px;

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

This text should drop down to font-size: 12px.

@@ -8,4 +8,8 @@
border-radius: $border-radius-s;
font-size: 13px;
}

&.Button--light {
@include button(#fff, $link-color);

This comment has been minimized.

Copy link
@mstriemer

mstriemer Mar 16, 2017

Member

You'll need to import the variables at the top of the file.

@import '~core/css/inc/vars';
@aayushsanghavi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

@mstriemer I'll read through the review after Saturday, if that's okay. I have college exams going on.
And thank you so much for taking the time out to review my patch as well as for your patience. Also, I apologize for making such stupid mistakes. I'll make sure that next time I thoroughly read through my code before I commit it. Thanks again. You're awesome!!

@mstriemer

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

Don't worry about it, it's looking good! Good luck with the exams.

@aayushsanghavi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

extensions
thmes
Current look of the extensions and themes home page

@tofumatt tofumatt closed this in cbc8402 Mar 24, 2017

@aayushsanghavi aayushsanghavi deleted the aayushsanghavi:layout_themes_home branch Mar 24, 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.