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 theme module layout #1872

Merged
merged 2 commits into from Feb 23, 2017

Conversation

muffinresearch
Copy link
Contributor

@muffinresearch muffinresearch commented Feb 22, 2017

Fixes mozilla/addons#10105

Fixes the theme layout - I made an assumption that we'd want to keep ratings. I also didn't look at the hyphenating of the text since that's covered elsewhere and needs discussion anyway.

Before:

screen shot 2017-02-22 at 21 41 25

After:

screen shot 2017-02-22 at 21 41 11

I also added some belt and braces fallbacks if images aren't set. End users are unlikely to see this but it covers the eventuality and we already did this on the details page for icons.

It will need extending for themes details pages at some point too but that could be done when revisiting the functionality for trying on a theme.

Extensions:

screen shot 2017-02-22 at 19 51 43

Themes:
screen shot 2017-02-22 at 19 46 54

width: 100%;

[dir=rtl] & {
object-position: top left;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing, I'm not sure if a real RTL browser will show a theme back to front? If so we should transform the images and do the same thing in the details page too.

Copy link
Contributor

@tofumatt tofumatt 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

Awesome, thanks so much for getting to this; looks great.

Not sure about the theme direction but if you wanted to merge this and open an issue for that sounds good. Don't think it blocks release anyway.

@@ -84,4 +86,42 @@ describe('<SearchResult />', () => {
assert.equal(node.textContent,
`Rated ${fakeAddon.ratings.average} out of 5`);
});

it('Displays a placeholder if the icon is malformed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The nit-pickiest of all nitpicks but the "D" should be lowercased here.

});


it('Adds theme specific class', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above; our style has been to use lowercase for the test descriptions.

@muffinresearch muffinresearch merged commit 6137924 into mozilla:master Feb 23, 2017
@muffinresearch muffinresearch deleted the fix-theme-module-layout branch February 23, 2017 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants