Bugfix/issue7 #11
Bugfix/issue7 #11
Conversation
anton-karlovskiy
force-pushed
the
bugfix/issue7
branch
from
8d13692
to
79c3188
Compare
There was a problem hiding this comment.
- Love the direction on this demo so far
- README LGTM
- Can we change the current output in https://www.dropbox.com/s/njov9dyw1ekw98y/issue7-overview.PNG?dl=0 to say "effective network connection" instead of "Network Connection"?
| import './App.css'; | ||
| import IFixitSearch from './containers/IFixitSearch'; |
There was a problem hiding this comment.
Can we change this to iFixitSearch?
There was a problem hiding this comment.
I'm afraid it looks like it's sort of rules in React component naming.
https://www.dropbox.com/s/7kc6g9x0lm9wf49/2.PNG?dl=0
Let me try to find warning ignore comment for this.
There was a problem hiding this comment.
Ah. Not a huge deal then :)
| </a> | ||
| </header> | ||
| <div className='App-content'> | ||
| <IFixitSearch /> |
There was a problem hiding this comment.
Similar comment to earlier (iFixitSearch)
There was a problem hiding this comment.
The same reason as above.
There was a problem hiding this comment.
Gotcha. Similar to above, I'd say we can skip this change then.
Yes, I did so. |
|
|
||
| import './search-bar.css'; | ||
|
|
||
| const SearchBar = ({ search }) => { |
There was a problem hiding this comment.
Do we currently throttle how often we query the API? We probably want to avoid doing this onChange for every keystroke, right?
There was a problem hiding this comment.
Now, we just call API once clicking on search icon, not calling API in every onChange event.
In every onChange event, we just update the input value.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks, let me check and update the codes.
There was a problem hiding this comment.
Ah, okay - so testing out https://anton-karlovskiy-network-aware-data-fetching-for-glitch.glitch.me/ I see that you need to tap on the magnifying glass in order to search (vs it searching on key-press). Rather than change this, maybe we can add the ability to search if you hit the ENTER key too?
There was a problem hiding this comment.
No problem. :)
There was a problem hiding this comment.
I added Enter key feature. You can check it on glitch version as I updated it as well.
| .then(response => response.json()) | ||
| .then(response => { | ||
| setLoading(false); | ||
| console.log('ray : response => ', response); |
There was a problem hiding this comment.
Could we drop the ray logs?
|
Milica advised me to include glitch in PR. |
Following is thumbnails in different resolution:
https://d3nevzfk7ii3be.cloudfront.net/igi/sX5iMoIvYMbCvHoF.thumbnail
https://d3nevzfk7ii3be.cloudfront.net/igi/sX5iMoIvYMbCvHoF.huge