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

remove the thumb icons from the homepage #2207

Merged
merged 8 commits into from
Nov 16, 2018
Merged

remove the thumb icons from the homepage #2207

merged 8 commits into from
Nov 16, 2018

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Nov 15, 2018

Closes ##2197

@Pomax Pomax requested a review from kristinashu November 15, 2018 22:26
@Pomax
Copy link
Contributor Author

Pomax commented Nov 15, 2018

Now that we have a min-seal, removing the thumbs makes products without a seal look ... off center? Maybe that's not a problem, but something to look at before we decide to land this.

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-2207 November 15, 2018 22:28 Inactive
@kristinashu
Copy link

I see what you mean. This PR is good but would need to revisit mini seal PR. Do you have to add that extra gray bar? I think the seal can fit within the gray border that surrounds the entire image.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2207 November 15, 2018 22:52 Inactive
@Pomax
Copy link
Contributor Author

Pomax commented Nov 15, 2018

PR updated, we'll have to see what it looks like with some actual images on staging =)

Copy link

@kristinashu kristinashu left a comment

Choose a reason for hiding this comment

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

Ah cool! Might need to make product images a little smaller but let's see.

@Pomax Pomax force-pushed the remove-home-thumbs branch from ffc0a24 to 5b00491 Compare November 15, 2018 23:09
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2207 November 15, 2018 23:10 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2207 November 15, 2018 23:26 Inactive
@Pomax
Copy link
Contributor Author

Pomax commented Nov 15, 2018

I've update the PR so that when filtering for likely/unlikely, the thumbs are revealed again, and default/filter:both hides them integrally.

@Pomax Pomax requested review from mmmavis and kristinashu November 15, 2018 23:28
@Pomax
Copy link
Contributor Author

Pomax commented Nov 15, 2018

@mavis added you for code review, since this PR also updates how the filter works: as long as the filter is set to "both" for buying likelihood, there will not be any thumb up/down icons shown. When someone picks "likely" or "not lilkely", the appropriate icon is shown on all the resultant products.

Also since it's technically possible to not have a recommendation HTML element, I added an if to make sure that if the queryselector fails, the filter doesn't break and disappear =)

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2207 November 15, 2018 23:32 Inactive
Copy link

@kristinashu kristinashu left a comment

Choose a reason for hiding this comment

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

Nice! That works great.

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

👍

@mmmavis mmmavis merged commit ced77e1 into master Nov 16, 2018
@mmmavis mmmavis deleted the remove-home-thumbs branch January 28, 2019 23:49
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