search

numberscope / frontscope

Numberscope's front end and user interface: responsible for specifying sequences and defining and displaying visualizers
MIT License
7 stars 15 forks source link

feat: Operational OEIS search bar #410

Closed gwhitney closed 3 months ago

gwhitney commented 4 months ago

By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.

This commit makes the OEIS search bar operational, and reconfigures it according to group discussions. I don't think we ever discussed the details of the presentation of the returned results themselves, so I am very much open to feedback and suggestions on the workings of the bar.

This is the final planned PR for the sake of cleaning/merging #360, and as such, supersedes that PR.

katestange commented 3 months ago

First main feedback: The mouse is a "cursor" mouse over the part you need to click on to load the sequence. It turns into a pointer finger only on the link to the OEIS url. So I was confused trying to figure out where I can load the sequence. Is this related to the fact that I get a Failed to load resource: the server responded with a status of 404 (Not Found) http://localhost:5173/favicon.ico error in the console?

katestange commented 3 months ago

Another small observation: There's no way to "back out" of a search except for deleting everything in the search box.... I kind of expected clicking somewhere else or something to back out. I'm not sure if this is what you had in mind or not, and I'm not sure what is the best behaviour, but I thought I would mention that it caused me a little bit of "exploration click time" that didn't result in anything.

gwhitney commented 3 months ago

Is this related to the fact that I get a Failed to load resource: the

No, it's just that I need to add the CSS command to change the cursor. Will do as soon as I have the opportunity.

gwhitney commented 3 months ago

observation: There's no way to "back out" of a search

Happy to implement other "back out" gesture(s) -- but just let me know what would be the intuitive action(s) to back out.

gwhitney commented 3 months ago

No, it's just that I need to add the CSS command to change the cursor. Will do as soon as I have the opportunity.

Yes, that was literally one line, which is now committed.

By the way, I meant to ask: should we bolden or highlight in some other way occurrences of the search string in the displayed names of the search results?

gwhitney commented 3 months ago

I don't think I ever mentioned that what's now the second-most-recent commit here is supposed to fix #401. Or rather, #403, but I think/hope it will also fix #401.

katestange commented 3 months ago

observation: There's no way to "back out" of a search

Happy to implement other "back out" gesture(s) -- but just let me know what would be the intuitive action(s) to back out.

I came back to it and played with it again today and I found myself just trying to click anywhere outside the search box/results area. I think that would be a perfectly fine solution. Then one thing to make sure is that clicking back in the search box, even without further typing, should make the results open again.

gwhitney commented 3 months ago

I found myself just trying to click anywhere outside the search box/results area. I think that would be a perfectly fine solution. Then one thing to make sure is that clicking back in the search box, even without further typing, should make the results open again.

I was trying to implement this but found I had a question: Right now, when the search results box is popped up, clicking on one of the still-visible sequence preview cards closes the search results and the Change Sequences modal and switches to the clicked sequence (even if that card is only partly visible, but the click is outside the search results. Should that behavior persist, OR while the search results is up, do you want a click outside the search results to close the search results and not do anything else? Thanks for letting me know.

katestange commented 3 months ago

I was trying to implement this but found I had a question: Right now, when the search results box is popped up, clicking on one of the still-visible sequence preview cards closes the search results and the Change Sequences modal and switches to the clicked sequence (even if that card is only partly visible, but the click is outside the search results. Should that behavior persist, OR while the search results is up, do you want a click outside the search results to close the search results and not do anything else? Thanks for letting me know.

My reaction is that the latter would be less confusing. But you can go with what seems to make sense to you.

gwhitney commented 3 months ago

OR while the search results is up, do you want a click outside the search results to close the search results and not do anything else? Thanks for letting me know.

My reaction is that the latter would be less confusing. But you can go with what seems to make sense to you.

OK, I have committed code intended to result in this latter behavior. You can pull and try it out at your leisure.

gwhitney commented 3 months ago

All right, I think that I've done everything open on this PR. From the latest commit message:

Of course if you see anything else that needs changing/fixing or if there is anything in the code that needs to be improved, I am happy to develop this PR further. Thanks!

gwhitney commented 3 months ago

P.S. In particular I don't love the phrasing I came up with for an "unsuccessful" search, namely

...... search for foobar gave no/too many results

So if anyone has better/clearer wording for this that is still relatively short, I would be happy to change.

katestange commented 3 months ago

Two comments:

(1) clicking on magnifier still gives me a tooltip and stops search

(2) for the message, you could try '....... search for foobar failed (OEIS reports no results or too many results)' One advantage of this phrasing is it attempts to pass the blame for the ambiguity ;) But simpler may be '....... search for foobar failed (no results or too many results)'

Everything else works great!

gwhitney commented 3 months ago

OK, I think I've taken care of both of those things.

katestange commented 3 months ago

When the search doesn't come back (axios/network/cors error) currently there's a popup and all interaction is interrupted for the user... but I think in the context of search especially, we should just continue (the same as if the user clicks "ok" on the popup) because it generally resolves itself at that point. At least this is what's happening for me right now. This might be appropriate to this PR or it might be something for post delft todos discussion? Ideas?

katestange commented 3 months ago

OK, I think I've taken care of both of those things.

I don't think you pushed the commits.

gwhitney commented 3 months ago

I don't think you pushed the commits.

Oops, embarrassing. I have now.

katestange commented 3 months ago

I don't think you pushed the commits.

Oops, embarrassing. I have now.

Ok, it looks good now. The tooltip pops up mouseover for the magnifier, which is good. Clicking causing search is essentially unverifiable at the moment for me, since my search is returning fast enough on typing (meaning, by the time I move my mouse and click, the search already fired). But that's ok, I don't see any suggested change.

I think the only outstanding issue is the popups on network errors (comment earlier today), which maybe should be disabled during search? Or maybe even entirely. Those popups haven't served a purpose for me other than interrupting things that later fix themselves.

gwhitney commented 3 months ago

but I think in the context of search especially, we should just continue (the same as if the user clicks "ok" on the popup) because it generally resolves itself at that point. At least this is what's happening for me right now. This might be appropriate to this PR or it might be something for post delft todos discussion? Ideas?

OK, I will create and push a commit that catches network errors in the OEIS search and adds a message that the search was temporarily unavailable, please try again, and will not cache that outcome.

katestange commented 3 months ago

but I think in the context of search especially, we should just continue (the same as if the user clicks "ok" on the popup) because it generally resolves itself at that point. At least this is what's happening for me right now. This might be appropriate to this PR or it might be something for post delft todos discussion? Ideas?

OK, I will create and push a commit that catches network errors in the OEIS search and adds a message that the search was temporarily unavailable, please try again, and will not cache that outcome.

Oh perfect, that sounds like a good solution.

gwhitney commented 3 months ago

OK, I have added such a commit. I will be honest though: I spent a while mashing the keyboard and could not invoke the new error-handling code. Maybe the time of day? (Less load on OEIS server?) So let me know how it works for you when you try it.

katestange commented 3 months ago

OK, I have added such a commit. I will be honest though: I spent a while mashing the keyboard and could not invoke the new error-handling code. Maybe the time of day? (Less load on OEIS server?) So let me know how it works for you when you try it.

Ok, I just tried it and was able to provoke a server error in the console that was not popped up and not apparent in the browser/user experience. So that's perfect. I don't know why I'm better at making the server mad. :)

katestange commented 3 months ago

Ok, I believe that everything is in order now with this, shall I squash and merge?

gwhitney commented 3 months ago

You're the reviewer ;-) anyhow, fine by me if you squash and merge -- I don't know of anything else outstanding.