Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

Feature/search #235

Merged
merged 1 commit into from
Jul 14, 2017
Merged

Feature/search #235

merged 1 commit into from
Jul 14, 2017

Conversation

motleydev
Copy link
Contributor

What: Adding style touches to #219

Why: Because style makes everyone happy. And this feature rocks.

How: Added some new primitives to the GlobalStyles object for animation and the rest was modifying the styles/structure of the Search component.

@codecov
Copy link

codecov bot commented Jul 13, 2017

Codecov Report

Merging #235 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
+ Coverage   95.31%   95.35%   +0.04%     
==========================================
  Files          31       31              
  Lines         320      323       +3     
  Branches       40       40              
==========================================
+ Hits          305      308       +3     
  Misses         15       15
Impacted Files Coverage Δ
components/nav.js 86.66% <100%> (+3.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 298acf0...3a86c75. Read the comment docs.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is looking soooo great! Thanks for the work and collaboration @RIanDeLaCruz and @motleydev! 👏

package.json Outdated
"react-dom": "^15.5.4",
"react-instantsearch": "^4.0.7",
Copy link
Owner

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RIanDeLaCruz Will need to comment on this.

Choose a reason for hiding this comment

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

react-instantsearch can be removed already. I find it odd that it wasn't removed when I did yarn remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to do some manual merges with master and this branch so it's possible I fat-fingered it.

},
})
const glamorousSearch = (props, {animations}) => ({
...animations.expansion('width'),
Copy link
Owner

Choose a reason for hiding this comment

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

Don't worry about the ..., also, I don't think we'll need to use this anywhere else so I'd prefer to keep this out of theme. So why don't we make those as part of inputBase. In addition, we could also just inline that, I don't think it needs to be in a variable. Why don't we just do:

const SearchBox = glamorous.input(
  'algolia_searchbox',
  { /* inline the styles object here */ }
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this library's functional style and ease of composing were the core features of the blog post I was thinking about, but ok. :) Done.

paddingBottom: 4,
paddingLeft: 30,
paddingRight: 20,
backgroundImage: 'url(/static/images/search.svg)',
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of making this external, would it be possible to just import the svg directly? We have a babel plugin that will give us a react component when we import an svg file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need to restructure the search box first. It's currently just an input but to place inline graphical elements like that we'd need to do some kind of span > input foo and hope it doesn't break the live results function. Do you still want to do that?

Choose a reason for hiding this comment

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

Pretty sure it should still be fine because docsearch just looks for the class name @motleydev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright @kentcdodds - you can decide if it's still worth the server response optimization. :) I won't be able to have a go at that until this evening most likely.

type: 'text',
pattern: '.*',
required: true,
placeholder: 'Search…',
Copy link
Owner

Choose a reason for hiding this comment

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

We'll need this to be localized :) Put it in components/content/nav.md and then you can do: placeholder: content.search

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kentcdodds kentcdodds mentioned this pull request Jul 14, 2017
@motleydev
Copy link
Contributor Author

Alright, requested changes are done except for the inline of svg - let me know!

@kentcdodds
Copy link
Owner

Ok, so I've made a few changes and cleaned up the commit history. Things are a little bit broken with the algolia config and that's because this was merged but I don't think it's been propogated yet. I think that I'll go ahead and merge this and hopefully it magically starts working when algolia reindexes things (tomorrow I'm guessing).

Repository owner deleted a comment from motleydev Jul 14, 2017
Repository owner deleted a comment from RIanDeLaCruz Jul 14, 2017
@kentcdodds kentcdodds force-pushed the feature/search branch 3 times, most recently from 58808c7 to a8e1207 Compare July 14, 2017 17:31
Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Ok, it's ready!

@kentcdodds kentcdodds merged commit d1ec291 into kentcdodds:master Jul 14, 2017
@kentcdodds
Copy link
Owner

Thanks everyone! This was a great team effort!

giphy

@motleydev
Copy link
Contributor Author

+1 for cleaning up the merge history. This one got hairy. :)

@Haroenv
Copy link
Contributor

Haroenv commented Jul 28, 2017

This usage of babel-plugin-preval is amazing!

@kentcdodds
Copy link
Owner

😊

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

Successfully merging this pull request may close these issues.

4 participants