-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
intermix prefetched usernames and API responses, de-duplicating and c… #9820
Conversation
f3b1487
to
64c3146
Compare
Codecov Report
@@ Coverage Diff @@
## main #9820 +/- ##
=======================================
Coverage ? 26.03%
=======================================
Files ? 98
Lines ? 7620
Branches ? 0
=======================================
Hits ? 1984
Misses ? 5636
Partials ? 0 |
Great justification - awesome! Looking now! |
This looks super!
Do you think this is a problem? And if not, do you think this is ready to merge? |
Finally, is there a system test you'd like to add to this, which would protect the functionality? It could search for the |
And, it would be nice to have it in the same PR, so that the test and the functionality are linked in the history! |
I'm still looking through At.js docs and issues for how to override the remoteFilter. For now, I think the merge can wait. |
I'm definitely going to write a test. I just needed to confirm I was going in the right direction. |
There's an open issue about the default filtering in ichord/At.js#448 but I've not found a solution. I tried changing the value of |
4d1bd5e
to
82cdaea
Compare
Hi @TildaDares 👋🏾 , looking at this now, you can respond tomorrow. What makes a user marked as recently active? should something be done with the users in the test case to make one active? Also, something else that could be an option is maybe adding a few seconds with before checking the assertion, with sleep or something else I have seen in the tests is a function called |
@RuthNjeri I've tried the |
af83c22
to
82cdaea
Compare
…lient-side filtering
82cdaea
to
3a38168
Compare
Code Climate has analyzed commit 3a38168 and detected 0 issues on this pull request. View more on Code Climate. |
Hi @TildaDares, I am not sure that there is a way to access the tmp/screenshot, currently I don't see any artifacts stored in the workflow There is a process to enable it, maybe @jywarren can have a look at it? (the process requires access to the Repo settings), https://docs.github.com/en/github/administering-a-repository/managing-repository-settings/configuring-the-retention-period-for-github-actions-artifacts-and-logs-in-your-repository I see you've updated the test to check for the list of names instead of the |
Great problem solving and yes, i'll look at the artifacts! We never had time to get those working in GitHub Actions yet. It'd be great! |
OK, cool, i opened a PR for the screenshot saving here, let's see: #9868 |
…lient-side filtering (publiclab#9820)
…lient-side filtering (publiclab#9820)
…lient-side filtering (publiclab#9820)
Part of larger planning issue #9667
Blending the list on the server side would be complex and inflexible and since both APIs return different JSON objects manipulating their objects would be a lot harder which is why I chose to do it on the client side.
No matter how I order the arrangement of both lists, the callback in the
remoteFilter
function always displays the usernames that match the first strings in the username at the top. In the video, you'll see this demonstrated when I type@a
.intermix.mov
You can test it here https://unstable.publiclab.org/