Enhance routing error html page #14619

Merged
merged 3 commits into from Apr 11, 2014

Conversation

Projects
None yet
4 participants
@winston
Contributor

winston commented Apr 6, 2014

This PR improves two things on the routing error html page:

Implement fuzzy matching for route search on routing error html page

Usually, when I hit the routing error html page, I would try to use the search field to find the route,
but the current search is pretty strict, and it doesn't do any "full text search".

So if I have the following routes, and I do a search for "profile", these routes wouldn't appear as results.

  • /users/:id/profiles/:id(.:format)
  • /users/:id/profiles/new(.:format)
  • /users/:id/profiles/:id/edit(.:format)

With the change, doing a search for "profiles" would return the above routes as results.

Improve CSS styling for routing error html page

Improve the CSS stylings for the routing error html page.

  • even rows highlight
  • larger paddings within table tds
@winston

This comment has been minimized.

Show comment
Hide comment
@winston

winston Apr 6, 2014

Contributor

@schneems Would love your comments, since you are the creator of the routing error html page.

Thanks!

Contributor

winston commented Apr 6, 2014

@schneems Would love your comments, since you are the creator of the routing error html page.

Thanks!

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Apr 7, 2014

Member

Can you also post the before/after pages after your style changes so we can take a look? Thanks.

Can you also post the before/after pages after your style changes so we can take a look? Thanks.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Apr 7, 2014

Member

Seems fine for profile to match the routes you listed, though I would also want as the path gets more complete for the list to get shorter.

Member

schneems commented Apr 7, 2014

Seems fine for profile to match the routes you listed, though I would also want as the path gets more complete for the list to get shorter.

@winston

This comment has been minimized.

Show comment
Hide comment
@winston

winston Apr 7, 2014

Contributor

@carlosantoniodasilva Adding screenshots for before and after.

@schneems The list does gets shorter as the path gets more complete, i.e. original functionality is still retained. I hope the screenshots help to explain how it would work.

Thank you!


"before" screenshot

screen shot 2014-04-07 at 11 06 30 pm


"after" screenshots.

Table with odd even rows highlight

screen shot 2014-04-07 at 10 57 15 pm

Fuzzy search

screen shot 2014-04-07 at 10 57 30 pm

Fuzzy search (more)

screen shot 2014-04-07 at 10 58 43 pm

Complete path

screen shot 2014-04-07 at 11 02 12 pm

Contributor

winston commented Apr 7, 2014

@carlosantoniodasilva Adding screenshots for before and after.

@schneems The list does gets shorter as the path gets more complete, i.e. original functionality is still retained. I hope the screenshots help to explain how it would work.

Thank you!


"before" screenshot

screen shot 2014-04-07 at 11 06 30 pm


"after" screenshots.

Table with odd even rows highlight

screen shot 2014-04-07 at 10 57 15 pm

Fuzzy search

screen shot 2014-04-07 at 10 57 30 pm

Fuzzy search (more)

screen shot 2014-04-07 at 10 58 43 pm

Complete path

screen shot 2014-04-07 at 11 02 12 pm

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Apr 7, 2014

Member

looks good to me. What happens when we search for users do we get everything that starts with /users?

Member

schneems commented Apr 7, 2014

looks good to me. What happens when we search for users do we get everything that starts with /users?

@winston

This comment has been minimized.

Show comment
Hide comment
@winston

winston Apr 8, 2014

Contributor

Yes. You'll get everything that starts with /users. That's the side effect of the "fuzzy match".

screen shot 2014-04-08 at 10 55 13 am

Contributor

winston commented Apr 8, 2014

Yes. You'll get everything that starts with /users. That's the side effect of the "fuzzy match".

screen shot 2014-04-08 at 10 55 13 am

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Apr 8, 2014

Member

hmm, so if you put in a route for sers it would show that several match
when they do not, that would be confusing to a new student. What if we
separate up matches so that we show exact matches on top, fuzzy matches
below and the rest below that. So in this exact same example we would get
GET and POST /users at top and everything else below that. It could also
say "No exact matches found for 'sers'" and only show fuzzy matches below.
I think both fuzzy and exact matches are a great feature to show, so maybe
separating them out and showing each as a seperate result would be good.
What do you think?

On Mon, Apr 7, 2014 at 9:57 PM, Winston notifications@github.com wrote:

Yes. You'll get everything that starts with /users. That's the side
effect of the "fuzzy match".

[image: screen shot 2014-04-08 at 10 55 13 am]https://cloud.githubusercontent.com/assets/2112/2639469/58936fb4-bec9-11e3-8312-837a3ed2dcde.png

Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/14619#issuecomment-39807074
.

Member

schneems commented Apr 8, 2014

hmm, so if you put in a route for sers it would show that several match
when they do not, that would be confusing to a new student. What if we
separate up matches so that we show exact matches on top, fuzzy matches
below and the rest below that. So in this exact same example we would get
GET and POST /users at top and everything else below that. It could also
say "No exact matches found for 'sers'" and only show fuzzy matches below.
I think both fuzzy and exact matches are a great feature to show, so maybe
separating them out and showing each as a seperate result would be good.
What do you think?

On Mon, Apr 7, 2014 at 9:57 PM, Winston notifications@github.com wrote:

Yes. You'll get everything that starts with /users. That's the side
effect of the "fuzzy match".

[image: screen shot 2014-04-08 at 10 55 13 am]https://cloud.githubusercontent.com/assets/2112/2639469/58936fb4-bec9-11e3-8312-837a3ed2dcde.png

Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/14619#issuecomment-39807074
.

@winston

This comment has been minimized.

Show comment
Hide comment
@winston

winston Apr 9, 2014

Contributor

I think it's a great idea to separate the two types of results - like a simple sort for most relevant on top.

I'll work on it.

Contributor

winston commented Apr 9, 2014

I think it's a great idea to separate the two types of results - like a simple sort for most relevant on top.

I'll work on it.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Apr 9, 2014

Member

Thanks! I'm really excited about this PR. Thank you for helping :)

On Wed, Apr 9, 2014 at 8:54 AM, Winston notifications@github.com wrote:

I think it's a great idea to separate the two types of results - like a
simple sort for most relevant on top.

I'll work on it.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/14619#issuecomment-39966021
.

Member

schneems commented Apr 9, 2014

Thanks! I'm really excited about this PR. Thank you for helping :)

On Wed, Apr 9, 2014 at 8:54 AM, Winston notifications@github.com wrote:

I think it's a great idea to separate the two types of results - like a
simple sort for most relevant on top.

I'll work on it.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/14619#issuecomment-39966021
.

@winston

This comment has been minimized.

Show comment
Hide comment
@winston

winston Apr 10, 2014

Contributor

Added another commit to the PR which splits the results into 'exact matches' and 'fuzzy matches'. Screenshots below. Let me know what you think! Thank you!

Default look and feel.

screen shot 2014-04-10 at 10 32 10 pm

Search that doesn't match anything.

screen shot 2014-04-10 at 10 32 17 pm

No exact matches. Some fuzzy matches.

screen shot 2014-04-10 at 10 32 26 pm

screen shot 2014-04-10 at 10 32 47 pm

Some exact matches. Some fuzzy matches.

screen shot 2014-04-10 at 10 32 33 pm

Some exact matches. No fuzzy matches.

screen shot 2014-04-10 at 10 32 40 pm

screen shot 2014-04-10 at 10 33 05 pm

Contributor

winston commented Apr 10, 2014

Added another commit to the PR which splits the results into 'exact matches' and 'fuzzy matches'. Screenshots below. Let me know what you think! Thank you!

Default look and feel.

screen shot 2014-04-10 at 10 32 10 pm

Search that doesn't match anything.

screen shot 2014-04-10 at 10 32 17 pm

No exact matches. Some fuzzy matches.

screen shot 2014-04-10 at 10 32 26 pm

screen shot 2014-04-10 at 10 32 47 pm

Some exact matches. Some fuzzy matches.

screen shot 2014-04-10 at 10 32 33 pm

Some exact matches. No fuzzy matches.

screen shot 2014-04-10 at 10 32 40 pm

screen shot 2014-04-10 at 10 33 05 pm

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Apr 10, 2014

Member

I love this, think it looks great ❤️ Tests are failing can you git pull --rebase git@github.com:rails/rails.git master and then push? You good @carlosantoniodasilva ?

Member

schneems commented Apr 10, 2014

I love this, think it looks great ❤️ Tests are failing can you git pull --rebase git@github.com:rails/rails.git master and then push? You good @carlosantoniodasilva ?

@winston

This comment has been minimized.

Show comment
Hide comment
@winston

winston Apr 11, 2014

Contributor

Thank you! :) Done with the rebase. Not sure why the specs were failing before though - had a look and it didn't seem to be anything related.

Contributor

winston commented Apr 11, 2014

Thank you! :) Done with the rebase. Not sure why the specs were failing before though - had a look and it didn't seem to be anything related.

rafaelfranca added a commit that referenced this pull request Apr 11, 2014

rafaelfranca added a commit that referenced this pull request Apr 11, 2014

@rafaelfranca rafaelfranca merged commit 14391d7 into rails:master Apr 11, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment