-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
create tableString for search fix #582 #824
Conversation
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.
Thanks for the contribution!
I would prefer a more functional approach for the logic using .map instead of .forEach.
I think this is what you were looking for 1b9710a |
It doesn't create the exact same string but I think its just fine for searching to create rather than | cell | cell | cell | cell | cell | cell | |
Anything else you need me to do? |
Can you please add a test case. I've tried your changes locally but it doesn't work. |
Where do I add a test case? sorry for my ignorance. |
Any update on this? This feature is a huge improvement for usability. |
im using it already. i just dont know where to write tests |
Any update on this? |
I think it can be merged. I'm not sure what else has to be done. |
As @timaschew mentioned you should add unit tests to validate your changes and to ensure they remain intact over time. I can try to help later today if you’re having trouble writing tests for the added functionality. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Can you please provide some snapshots for these changes. I cant review this as we don't have tests to validate this. |
@anikethsaha Hello, can this pr fix #582 ? I also encountered this problem, or does the official have time to fix it? |
I'd also like to see support for searching table contents. If you need some test content, perhaps you can look at our documentation? https://docs.liveryvideo.com/web-sdk Let me know if/how I can help. |
closed by #1015 |
the PR is merged with this fix, |
Create Table String for Search
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.
Other information:
lib
directory.