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

Add collection label next to search results #1068

Merged
merged 11 commits into from
Feb 7, 2018
Merged

Add collection label next to search results #1068

merged 11 commits into from
Feb 7, 2018

Conversation

solpark
Copy link
Contributor

@solpark solpark commented Feb 1, 2018

- Summary
Closes issue #945
Collection name is indicated on each search result item.

- Test plan
Shows collection name in each search result:
netlify-search-results

Does not show collection name next to items when not searched:
netlify-non-search-results

- Description for the changelog

Add collection label next to search results

- A picture of a cute animal (not mandatory but encouraged)
image

src: http://i.dailymail.co.uk/i/pix/2012/09/17/article-2204494-150E707A000005DC-700_634x438.jpg

@verythorough
Copy link
Contributor

verythorough commented Feb 1, 2018

Deploy preview for netlify-cms-www ready!

Built with commit f08535b

https://deploy-preview-1068--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Feb 1, 2018

Deploy preview for cms-demo ready!

Built with commit f08535b

https://deploy-preview-1068--cms-demo.netlify.com

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

Thanks @solpark!

I'll give a short review each for code and design.

Code

We can simplify things by drilling down to the abstract goal: whenever entries from multiple collections are displayed at once, regardless of why, we should label each entry with it's collection title. The EntryListing component has a renderCardsForMultipleCollections method that handles this specific case, so we can start there without altering components that are higher up in the tree (e.g. passing the isSearchResults flag around).

The renderCardsForMultipleCollections method wraps the EntryCard component directly, so we can simply pass in a label string when we want a label, and don't use the label prop when we don't. This approach should allow us to get away with only modifying two components total.

Design

The approach totally works, but I happen to know that there will eventually be labels or tags or who-knows-what in that right side area of the entry card, so it might not be ideal placement. There's also precedent for placing a UI element's label in the top left, such as in editor fields and editorial workflow cards, both pictured below:

screen shot 2018-02-02 at 4 19 21 pm

screen shot 2018-02-02 at 3 59 05 pm

That last example is very similar to the card you're modifying - the cards are from the "Blog" collection, so "BLOG" is indicated in the upper left corner of each. You can probably borrow those styles directly and maybe avoid writing new styles. Also like the last example, you can just use the collection name without prepending "Collection:".

Thanks again for taking the time to contribute! I hope the review was helpful - feel free to ask questions here or in Gitter.

@solpark
Copy link
Contributor Author

solpark commented Feb 3, 2018

@erquhart Thanks for the thorough feedback! I will make the changes ASAP :)

@solpark
Copy link
Contributor Author

solpark commented Feb 3, 2018

After changes:
netlify-pr-change

@solpark
Copy link
Contributor Author

solpark commented Feb 5, 2018

@erquhart
Hi! I made the changes but the netlify deploy-preview test is failing. I asked about this in Glitter and it seems to be a netlify issue. How should I go forward with this?

]
},
{
"login": "michaelromani",
Copy link
Contributor

@tech4him1 tech4him1 Feb 5, 2018

Choose a reason for hiding this comment

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

@solpark Can you confirm that adding "Michael Romani" is intentional? Just wanted to check. 😃

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, worked on this together!

@tech4him1
Copy link
Contributor

@solpark The build should be working now, Netlify was able to get the Yarn issue fixed.

@solpark
Copy link
Contributor Author

solpark commented Feb 5, 2018

Ah gotcha. Thank you!

@erquhart
Copy link
Contributor

erquhart commented Feb 7, 2018

Did a bit of editing, borrowed styles from the workflow cards:

screen shot 2018-02-07 at 4 29 50 pm

@erquhart
Copy link
Contributor

erquhart commented Feb 7, 2018

I think this is good to merge - thanks for taking it on @solpark!

Sol and others added 11 commits February 7, 2018 15:09
Co-authored-by: Mike Romani 29218846+MichaelRomani@users.noreply.github.com
Co-authored-by: Mike Romani 29218846+MichaelRomani@users.noreply.github.com
Co-authored-by: Mike Romani 29218846+MichaelRomani@users.noreply.github.com
Co-authored-by: Mike Romani 29218846+MichaelRomani@users.noreply.github.com
Co-authored-by: Mike Romani 29218846+MichaelRomani@users.noreply.github.com
@tech4him1 tech4him1 merged commit 1d41620 into decaporg:master Feb 7, 2018
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