H1: Replace Legacy Search #100
Conversation
Task/arxivng 186
Task/arxivng 282
…search results; fixed missing sort button
There was a problem hiding this comment.
My overall impression from experimenting with search service is very positive. I mostly experimented with functionality described in the README. I spent limited time looking at code but am impressed at what code I did examine.
A few minor comments.
I initially downloaded the arXiv-search repository expecting to run and review standalone on my laptop. Our discussions of isolation and mocking had me thinking I could experiment in isolation. Sadly, I soon found out that I needed permission to the production server to populate test database.
2018-03-12 17:05:30,356 - search.services.metadata - ERROR: Request failed: b'<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">\n<html>\n<head><title>403 Forbidden</title></head>\n<body>\n<h1>Access Denied</h1>\n\n<p>Sadly, you do not currently appear to have permission to access\n<b>https://arxiv.org/docmeta/0704.3437</b></p>\n\n\n<p>If you believe this determination to be in error, see\n<b>https://arxiv.org/denied.html</b> for additional information.</p>\n</body>\n</html>\n'
2018-03-12 17:05:30,356 - search.agent.consumer - ERROR: 0704.3437: request failed
Sorry Dave, I'm afraid I can't do that! No problem that my VPN would not fix.
The tests failed for me. Since I saw other reports of failure and apparent known problems I did not investigate further.
When I ran pylint it seemed like there were a number of trivial warnings to clean up. On the other hand you likely have already cleaned up a lot such that the 9.31/10 rating meets the desired threshold.
When making the documentation 'make latexpdf' I saw a few errors fly by that I wasn't expecting.
WARNING: autodoc: failed to import module 'search.exceptions'; the following exception was raised:
No module named 'search.exceptions'
WARNING: autodoc: failed to import module 'search.process.query'; the following exception was raised: No module named 'search.process.query'
My development search page seemed to work fine for the documents indexed.
The only search UI thing that bothers me ever so slightly (in Safari) is no box around the search text input area. So I need to mouse around to discover the box...the faint search terms helps...I guess it just bothers me when I need to mouse over to make input areas visible. Not sure how this impacts accessibility. I suppose auxiliary accessibility tags make it easier for someone with a page reader.
I will try to spend some more time on this and add additional comments.
|
@DavidLFielding Thank you for the note about contrast on the search box outlines! I'd been a bit worried about it after seeing the site on the projector in Room 236. Do you see a box at all around "Tips"? I am considering boosting contrast for both the box outlines and the helper text inside the boxes (would apply to universal styling for the site, so all instances of input fields throughout). |
|
I see no box around tips. I guess you need better contract between your white border and the white background. :^)
And I can barely see the faint 'Search terms...' text on my 24" monitor. You might want to stop by or I can take a screen shot.
I clicked on the search button in the header and get a 404 - I'm assuming this is not wired up at the moment. I'm able to move between simple search and advanced search in body of page.
…________________________________
From: eawoods <[email protected]>
Sent: Tuesday, March 13, 2018 11:09:42 AM
To: cul-it/arxiv-search
Cc: David L. Fielding; Mention
Subject: Re: [cul-it/arxiv-search] H1: Replace Legacy Search (#100)
@DavidLFielding<https://github.com/davidlfielding> Thank you for the note about contrast on the search box outlines! I'd been a bit worried about it after seeing the site on the projector in Room 236. Do you see a box at all around "Tips"? I am considering boosting contrast for both the box outlines and the helper text inside the boxes (would apply to universal styling for the site, so all instances of input fields throughout).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#100 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACN7iDEt24VG14FJAkNMkAUQ8YwSQD0Bks5td-E2gaJpZM4SjTl4>.
|
|
Noted - the box border colors for tips and for input are controlled by variables, and I'll just boost those. Not sure how bright they need to be since there's no requirement for layout elements, but should definitely be at least a little bit visible! |
|
@DavidLFielding Regarding the documentation errors: looks like we had some outdated RST source files for the API documentation. I've updated those just now. |
|
Liz,
Maybe I should check out deployed version. Yesterday I tried to use Beta and ran into internal server errors. Is it running on different port?
I was going to include screenshot but doesn't do justice to how light it is on my big monitor. I knew I should have purchased new glasses before the review.
David
…________________________________
From: eawoods <[email protected]>
Sent: Tuesday, March 13, 2018 11:22:38 AM
To: cul-it/arxiv-search
Cc: David L. Fielding; Mention
Subject: Re: [cul-it/arxiv-search] H1: Replace Legacy Search (#100)
Noted - the box border colors for tips and for input are controlled by variables, and I'll just boost those. Not sure how bright they need to be since there's no requirement for layout elements, but should definitely be at least a little bit visible!
404 on header search box - that happens to me also in my local environment, but it works when fully deployed. (@erickpeirson<https://github.com/erickpeirson> is that something we need to worry about or is it all OK?)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#100 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACN7iB0FheQsEgnS-ebxD1TQI5r9g85Fks5td-Q9gaJpZM4SjTl4>.
|
|
@DavidLFielding I'll come have a look at your monitor. In the meantime: http://arxiv.org/search-beta - not quite all the newest latest and greatest is deployed here but it was a staging exercise for the beta release. |
Add Flask-S3 plugin support and other minor changes
ARXIVNG-333 docker runtime for indexing agent
Review Process
General Expectations
What to look for
Feedback collection and format
Giving effective feedback