grid-js
Bugfix - Server side render #1081
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:
- Keep the 'input value' and 'search' in two separate places,
- Eliminate the need to keep the input value in the state,
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 😃
Sure 👍
On Sat, Aug 20, 2022, 9:23 PM Afshin Mehrabani @.***> wrote:
@.**** commented on this pull request.
In src/view/plugin/search/search.tsx https://github.com/grid-js/gridjs/pull/1095#discussion_r950684649:
@@ -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 https://github.com/grid-js/gridjs/pull/1095#pullrequestreview-1079513784, 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 https://github.com/grid-js/gridjs/pull/1095#discussion_r950684649:
@@ -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 https://github.com/grid-js/gridjs/pull/1095#pullrequestreview-1079513784, 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.
@afshinm @Aloysb - thank you so much for working on this and I'm sorry that I can't assist with this myself, but is there any chance we could get this merged in? This bug is making the server side search a bit of a poor experience for my users.
