-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Bugfix - Server side render #1081 #1095
base: master
Are you sure you want to change the base?
Conversation
@@ -113,7 +113,6 @@ export class Search extends PluginBaseComponent< | |||
className('input'), | |||
className('search', 'input'), | |||
)} | |||
value={this.store.state.keyword} |
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.
Could you please add a test for this please? I'm wondering why we don't need the keyword
value in the store
Sure 👍
…On Sat, Aug 20, 2022, 9:23 PM Afshin Mehrabani ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/view/plugin/search/search.tsx
<#1095 (comment)>:
> @@ -113,7 +113,6 @@ export class Search extends PluginBaseComponent<
className('input'),
className('search', 'input'),
)}
- value={this.store.state.keyword}
Could you please add a test for this please? I'm wondering why we don't
need the keyword value in the store
—
Reply to this email directly, view it on GitHub
<#1095 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEHYZ4RV7VTHODSDEINFDNLV2C54PANCNFSM56ZINMIA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hey, Afsh, how are you?
Did you get my message on Discord?
I'm having difficulties running the test out the box. Do you want me to
investigate this? Seems to be a dependency issue around the enzyme adapter.
I've actually never used Enzyme, always the testing-library but it seems
very similar.
Cheers,
Alo
…On Sun, 21 Aug 2022 at 05:21, Aloys Berger ***@***.***> wrote:
Sure 👍
On Sat, Aug 20, 2022, 9:23 PM Afshin Mehrabani ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/view/plugin/search/search.tsx
> <#1095 (comment)>:
>
> > @@ -113,7 +113,6 @@ export class Search extends PluginBaseComponent<
> className('input'),
> className('search', 'input'),
> )}
> - value={this.store.state.keyword}
>
> Could you please add a test for this please? I'm wondering why we don't
> need the keyword value in the store
>
> —
> Reply to this email directly, view it on GitHub
> <#1095 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEHYZ4RV7VTHODSDEINFDNLV2C54PANCNFSM56ZINMIA>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
*Aloys B.*
✉️ ***@***.***
🐱 AloysB (GitHub) <https://github.com/aloysB/aloysB>
✍️ A bit better than yesterday <https://abitbetterthanyester.day>
|
hey @aloysb, oh no, sorry I didn't see your message. I'm not usually online on Discord. Are you getting any errors when running the tests? I actually think it makes more sense to use the testing library instead of Enzyme, I did that for the gridjs-react wrapper last week. |
This PR fix the issue as described on #1081 🎉 🍻
There is not, as far as I can tell, a reason to have the search set up as a controlled input.
I've removed it, as it was creating a racing condition with the debouncing/async API call.
When the API returned, it would override the current input value by the search.
Coupled with the debouncing, it would create weird situations where depending on how fast you typed vs. how fast the server responded, some letters seemed to disappear.
There were two ways to go about it:
I went for 2 - Keep It Simple, Silly.
This solves issue #1081 🎉
There is no E2E set up for the search, so I'm pushing this as a hotfix - but we should think about bringing more E2E to the table.
================
On a side note,
@afshinm feel free to contact me. I'm happy to contribute regularly to grid-js - I've rolled out my own table package internally and I have come to realise how much work it is. 😪
I'd be happy to help out here: a nice language agnostic, pluggable table package is very much needed and it seems that Grid-js might need some TLC 🧽 ❤️ 💅
Cheers 😃