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

Addon detail touchups #1570

Merged
merged 7 commits into from
Jan 6, 2017
Merged

Conversation

mstriemer
Copy link
Contributor

@mstriemer mstriemer commented Jan 6, 2017

We had some old placeholders and things that were waiting on common elements and it's all ready now so I tried to make the detail pages match the mocks a little more closely.

@pwalm for the screenshots I tried out a few different versions, I think they all look pretty good but I'm leaning towards this one as it is more consistent with the other boxes. I linked the other two versions, let me know which you prefer and I'll go with that.

Add-on detail screenshot card
Add-on detail screenshot card with header
Add-on detail screenshot without card

Theme detail

Sorry the images are so big. You used to be able to specify a max-width but that appears to no longer be allowed 😞

@kumar303
Copy link
Contributor

kumar303 commented Jan 6, 2017

My Sketch file shows the 'read more' link a bit differently. Is mine out of date?

screenshot 2017-01-06 10 01 55

@tofumatt
Copy link
Contributor

tofumatt commented Jan 6, 2017 via email

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

I didn't test it out but here's some feedback on the code

@@ -1,37 +0,0 @@
import React, { PropTypes } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should delete this file yet. I know it has been pushed to V2 but Jorge requested that we at least implement user count for V1 since we do have that data available to us: https://docs.google.com/document/d/13g8tmoII8oCNrUXOl2HegsQst7WEsXukBnIqPUNrqb0/edit#heading=h.2ltzvz63pc6h

Also, I was planning to get the user count stuff done today :) https://github.com/mozilla/addons-frontend/issues/1293

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll leave that part in and let you touch it up. I think it might look a bit weird having that blue bar with only one thing in it. I'll move the install switch into the bar and then @pwalm can give us some feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, hmm. Maybe you can remove everything except for the user count and then it could sit on the left of the install switch.

@kumar303
Copy link
Contributor

kumar303 commented Jan 6, 2017

I thought it was a lot more obvious and usable as a larger and more prominent link.

works for me!

@mstriemer
Copy link
Contributor Author

Added users back in and pulled the switch into the blue bar. Also pulled the user count icon from the sketch file since it was black before.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

r+wc

Sweet, I like the way that looks. Merge it in and I'll hook up the user count.

background: url('../img/categories/security.svg') no-repeat 50% 5px;
}
.AddonMeta-users {
// background: url('../img/icons/users.svg') no-repeat 50% 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

can this just be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks.

@mstriemer
Copy link
Contributor Author

@pwalm feel free to comment here or file an issue with suggested changes.

@mstriemer mstriemer merged commit ea81870 into mozilla:master Jan 6, 2017
@mstriemer mstriemer deleted the addon-detail-touchups branch January 6, 2017 18:14
@pwalm
Copy link
Contributor

pwalm commented Jan 6, 2017

@mstriemer These screens look fantastic. A few things:

  • copy should be larger and white in the search bar
  • "by [developer name]" should be the same size as the extension name
  • let's use the "read more" button at the bottom of the "about this extension" block instead of the faded text under the link. We'll still keep the fade, but can we make it shorter? Like, 20px transition.
    screen shot 2017-01-06 at 3 52 41 pm
  • some body copy looks grey, it should all be #000000

One big, site wide thing: Can we change the blue header zone to dark grey? Things will clash less and the switch will stand-out better.
screen shot 2017-01-06 at 3 54 51 pm
top: #343A40
search bar: #495057

@mstriemer
Copy link
Contributor Author

copy should be larger and white in the search bar

This is already fixed on -dev.

"by [developer name]" should be the same size as the extension name

Sounds good

let's use the "read more" button at the bottom of the "about this extension" block instead of the faded text under the link. We'll still keep the fade, but can we make it shorter? Like, 20px transition.

I was thinking the same thing 👍 to consistency on these cards

some body copy looks grey, it should all be #000000

I can hardly tell the difference switching back and forth with black but it would appear you are correct 👏

One big, site wide thing: Can we change the blue header zone to dark grey? Things will clash less and the switch will stand-out better.

I'm sorry Phil, I'm afraid I can't do that. Technical reasons... I like the blue.

@kumar303
Copy link
Contributor

kumar303 commented Jan 7, 2017

Aww, I liked the blue as well.

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.

4 participants