-
Notifications
You must be signed in to change notification settings - Fork 132
@shopify/theme-predictive-search-component #96
Conversation
packages/theme-predictive-search-component/__tests__/theme-predictive-search-component.test.js
Outdated
Show resolved
Hide resolved
41f0951
to
334182f
Compare
442fda7
to
4ef0863
Compare
da5eadc
to
8c3bab9
Compare
const predictivesearch = new PredictiveSearch({ | ||
selectors: { | ||
input: '[data-predictive-search-input="header"]', | ||
reset: '[data-predictive-search-reset="header"]', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for a reset button is not completely fleshed out atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete line 11 then?
packages/theme-predictive-search-component/__tests__/theme-predictive-search-component.test.js
Outdated
Show resolved
Hide resolved
packages/theme-predictive-search-component/__fixtures__/search_as_you_type_the_following.json
Outdated
Show resolved
Hide resolved
packages/theme-predictive-search-component/__fixtures__/search_as_you_type_the_following.json
Outdated
Show resolved
Hide resolved
packages/theme-predictive-search-component/__tests__/theme-predictive-search-component.test.js
Outdated
Show resolved
Hide resolved
```js | ||
const predictivesearch = new PredictiveSearch({ | ||
selectors: { | ||
input: '[data-predictive-search-input="header"]', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show somewhere that these are required
selectors: { | ||
input: '[data-predictive-search-input="header"]', | ||
reset: '[data-predictive-search-reset="header"]', | ||
result: '[data-predictive-search-result="header"]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these selectors have assumptions that each is only returning 1 DOM element?
console.log(nodes.input.id); //-> input | ||
console.log(nodes.submit.id); //-> submit | ||
console.log(nodes.result.id); //-> result | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this confusing: Does this callback only fires when the input is focused? or does it callback when either input, submit, or result focuses?
What is the purpose to have this event proxy if it's only giving a callback on input focus? Could these be solved by simply having a bindEvent callback so that theme developers can do whatever they need to do, be it focus, keyup, blur, touch, ... etc browser events?
onBeforeOpen: nodes => {}, | ||
onOpen: nodes => {}, | ||
onBeforeClose: nodes => {}, | ||
onClose: nodes => {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these for animation events?
ba50c0e
to
c6ef76d
Compare
8c3bab9
to
d67c4d0
Compare
45a2421
to
7bbbf4f
Compare
a0ab60b
to
a51a98b
Compare
Please feel free to reach out if needed to ship this PR! 🙂 |
@antoinegrant A lot of questions and comments have not been addressed on the PR and, although the code here in this PR is used in Debut, and will ship with it within days, there seems to be mistrust in the work here. Also, the Predictive Search API has changed, this PR reflects the changes, and what is on |
@chrisberthe @t-kelly my understanding is that there might be some opposition to making this script public. Is this the case? We wanted to use this library to ship the new version of Debut with Predictive Search and to eventually spread it to other themes. |
Pretty sure there isn't any opposition on our side - where did you hear this? Plus, the script is technically already public. |
Thanks @chrisberthe, good to know. I think I misunderstood. |
b7ab286
to
ee723ca
Compare
538cfef
to
91108ef
Compare
91108ef
to
93b0e2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I can't comment from a coding perspective, I 🎩 and it's working great.
Thanks so much for all your help in getting this out the door! 🥇
Description
A component that controls the state of a predictive search UI.
Reviews
Reviewers should focus on this commit as this is a branch of
predictive-search
.Example
CC @NathanPJF @t-kelly @beefchimi