Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[WIP] Search update #12689

Closed
wants to merge 3 commits into from
Closed

Conversation

aaronmarkham
Copy link
Contributor

@aaronmarkham aaronmarkham commented Sep 27, 2018

Description

This PR improves version switching on the website. The new feature helps with searching for APIs mostly, but can also work for pages you're searching for that exist in different versions.

The current version selector will kick you to the home page from the search page. I fixed this behavior for the API section of the docs last week, but thought fixing search would be a big UX improvement too.

It's generalized now too.

  • It should work for any section of the site.
  • It should retain both bookmarks # and query strings ?.
  • It is overridden by the .htaccess file, so sections like tutorials will still always go to the latest.

UPDATE: To deal with requests to the root of the website, I updated the redirects to point to the default version. Right now this means any request to / will go to /versions/master. Or requests to /tutorials/xyz go to /versions/master/tutorials/xyz.

Usage

As this is a javascript update, please be sure to clear your cache.
Let's say you're looking for allreduce, but for whatever reason you're on the v1.0.0 of the site (like coming in from Google):
http://34.201.8.176/versions/1.0.0/search.html?q=allreduce&check_keywords=yes&area=default#
You get no results, but you're prompted to use the versions dropdown.
So you choose 1.3.0:
http://34.201.8.176/versions/1.3.0/search.html?q=allreduce&check_keywords=yes&area=default
You get a result.

Preview

2018-09-27_16-15-22

Comment

It would be awesome if the search fails we have it return other versions as suggestions, but I'm not that familiar yet with how Sphinx does its search magic... so that can be another day!

Test Plan

@safrooze found a hole in the regex coverage, so I think it is worth identifying the various angles of entry and usage.

@vandanavk
Copy link
Contributor

@mxnet-label-bot [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Sep 27, 2018
@@ -1,17 +1,17 @@
function setVersion(){
let doc = window.location.pathname.match(/^\/(api\/.*)$/) || window.location.pathname.match(/^\/versions\/[^*]+\/(api\/.*)$/);
let doc = window.location.pathname.match(/^\/versions\/[0-9.master]+\/([^*]+.*)$/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this fail if the version user is looking at is master, where /versions/ is missing from the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep... good catch... I was testing the other side of the use case.

I've been thinking about a related point. I could just force all the default traffic to /versions/master. This would solve a variety of thorny issues. Also, help with analytics. WDYT about that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think I'd personally like to see /versions/xyz in the URL all the time. In the future if we decide to change default to latest release rather than master, we can easily change the redirect and everything else should work as expected.

@sandeep-krishnamurthy
Copy link
Contributor

@safrooze @aaronmarkham - ping for next steps here

@aaronmarkham
Copy link
Contributor Author

@sandeep-krishnamurthy @safrooze I've re-enabled the preview. You should clear your cache to make sure you're getting the latest .htaccess file.
This was pretty challenging to resolve and I hope I have the different angles covered with:
https://github.com/apache/incubator-mxnet/pull/12689/files#diff-8752dbf83f90b837af0035f3c1291ecdR31
That should redirect traffic to the root to master. Requests to other versions go through. I added an env var to htaccess to hold the default version, which is currently 'master'.
I kind of wish I wasn't combining the search fix with this kind of structure fix, but anyway, here we are. Please test it out.

@aaronmarkham aaronmarkham changed the title Search update [WIP] Search update Oct 12, 2018
@ankkhedia
Copy link
Contributor

@safrooze Could you please take a look into this PR?

@aaronmarkham
Copy link
Contributor Author

I'm basically stuck on some thorny regex. That's why I renamed this PR with the [WIP]. I'd like to make the regex a little looser so it lets through any call to /versions/{anything}, and redirects any /{anything but versions}/ back to the default. I'd appreciate help.

The .htaccess file is always cached by the browser, so it makes testing a pain. Use incognito mode.

@kalyc
Copy link
Contributor

kalyc commented Nov 12, 2018

@aaronmarkham could you please elaborate more on the issue by providing an example? You can consider addressing the regex issue with another PR and have this PR move ahead.

@aaronmarkham
Copy link
Contributor Author

There are two competing routes to make this move forward.
The first is to add a second match to the regex so that anything /not-in-versions/ folder works, as mentioned by Sina.
The second, and the route I started to take here, is to make sure everything is in /versions/. That requires a redirect on anything not in versions. Smartly done, it'll take your request to /api and move it the /versions/master/api, where master is what is set to be the default in htaccess and any root requests are forwarded... but it's not working...:

RewriteRule .* - [E=default_version:/versions/master]
# other rules
...
# finally, check and forward
# Redirect any root requests to the default version
RewriteRule ^(?!versions\/[0-9.master]+\/)(.*)$ %{ENV:default_version}/$1 [R=301,NC,L]

@stu1130
Copy link
Contributor

stu1130 commented Nov 26, 2018

@aaronmarkham are you able to fix the issue you mentioned above?
If it will take you a long time, you can create a separate PR for this so that we can merge this one

@roywei
Copy link
Member

roywei commented Dec 11, 2018

@aaronmarkham ping for update, thanks!

@aaronmarkham
Copy link
Contributor Author

Closing this in favor of tackling the problems separately. The site should be updated to have traffic directed to the default version. Then we can improve search.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants