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

Update SearchBar to better match comps #2042

Merged
merged 12 commits into from
Feb 21, 2019

Conversation

pepopowitz
Copy link
Contributor

@pepopowitz pepopowitz commented Feb 20, 2019

This PR includes a variety of styling changes, with the goal of matching the auto-suggest search bar to the Zeplin comps.

Included:

  • Deal with white on white for the border of the suggest area (box-shadow)
  • Limit suggestions to 6 items, including the placeholder
  • Limit grid preview items to 10 items
  • Limit marketing collections to 6 items
  • We're missing the line between suggestions and previews
  • Fix positioning (in reaction)
  • Width hasn't grown into container properly (in reaction)
  • Match comp for search suggestions (height, for example)
  • Missing padding at top of suggestion/preview panes
  • Suggestion item text should have no hover underline
  • overflow: ellipsis for artist names (search for Fiona Banner, and you'll see)
  • Wrap preview images in a with static width/height to keep things from dancing while loading

Here's what it looks like now!

kapture 2019-02-20 at 20 19 18

@jonallured jonallured mentioned this pull request Feb 20, 2019
10 tasks
<ContextProvider>
<SearchBar />
</ContextProvider>
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, nice!

@artsyit
Copy link
Contributor

artsyit commented Feb 20, 2019

Deploy preview for artsy-reaction ready!

Built with commit 84b9595

https://deploy-preview-2042--artsy-reaction.netlify.com

@peril-staging
Copy link
Contributor

peril-staging bot commented Feb 20, 2019

Fails
🚫 Please assign someone to merge this PR, and optionally include people who should review.

Generated by 🚫 dangerJS against f711ccf

@peril-staging
Copy link
Contributor

peril-staging bot commented Feb 20, 2019

Fails
🚫 Please assign someone to merge this PR, and optionally include people who should review.

Generated by 🚫 dangerJS against f711ccf

@peril-staging
Copy link
Contributor

peril-staging bot commented Feb 20, 2019

Fails
🚫 Please assign someone to merge this PR, and optionally include people who should review.

Generated by 🚫 dangerJS against a2a52cc

@peril-staging
Copy link
Contributor

peril-staging bot commented Feb 20, 2019

Fails
🚫 Please assign someone to merge this PR, and optionally include people who should review.

Generated by 🚫 dangerJS against 848d7bc

@peril-staging
Copy link
Contributor

peril-staging bot commented Feb 20, 2019

Fails
🚫 Please assign someone to merge this PR, and optionally include people who should review.

Generated by 🚫 dangerJS against 2c85cb8

1 similar comment
@peril-staging
Copy link
Contributor

peril-staging bot commented Feb 20, 2019

Fails
🚫 Please assign someone to merge this PR, and optionally include people who should review.

Generated by 🚫 dangerJS against 2c85cb8

@pepopowitz pepopowitz changed the title WIP - Update SearchBar to match comps Update SearchBar to better match comps Feb 21, 2019
@pepopowitz pepopowitz requested a review from damassi February 21, 2019 02:21
@@ -42,12 +42,15 @@ export const PreviewGridItem: React.SFC<PreviewGridItemProps> = ({
</Link>
<Link href={artwork.href} noUnderline>
<Box>
<Title size="2">
<OverflowEllipsis size="2" italic>
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -11,31 +12,34 @@ interface Props {

export const SuggestionItem: SFC<Props> = ({ href, display, label, query }) => {
if (label === "FirstItem") {
// `color="black100"` is misleading.
// The actual color doesn't really matter - the child element controls the displayed color.
// We specify color only because it makes the text not underlined on hover.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it would make a good API improvement PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, definitely. We've been talking about this quirk in the <Link> component, and trying to figure out what to do with it. It meets the specs that were originally created in Zeplin, but it's confusing & misleading.

@damassi
Copy link
Member

damassi commented Feb 21, 2019

This thing is a beauty, great work 👍

@pepopowitz pepopowitz merged commit 8642e35 into artsy:master Feb 21, 2019
@pepopowitz pepopowitz deleted the search-bar-visuals branch February 21, 2019 19:14
@artsyit
Copy link
Contributor

artsyit commented Feb 21, 2019

🚀 PR was released in v14.0.13 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants