Skip to content

Improve no result page #1210

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

Merged
merged 7 commits into from
Jun 9, 2014
Merged

Improve no result page #1210

merged 7 commits into from
Jun 9, 2014

Conversation

oiami
Copy link
Contributor

@oiami oiami commented Jun 3, 2014

I've done mostly on No result and a little on result page. Changes are:

screenshot 2014-06-03 21 53 17
*Hopefully we can add more suggestion keywords using suggester from ES but this could happen after we upgrade ES to later version (0.90+)

screenshot 2014-06-03 21 52 29

First intention was trying to make font size larger on no result page but as a result of the changes I've made. I removed the text ( 0 results (0.xxx seconds) ) from no result page but this change still apply to result page. Which I think it nicer. (What do you think ? :D)

I made these changes based on my opinion, so you might not like it or have better idea. Any comments and suggestions are really welcome if you could give me to make MetaCPAN be brighter and better :)

@tsibley
Copy link
Contributor

tsibley commented Jun 3, 2014

These screenshots look great! @oiami++

@oalders
Copy link
Member

oalders commented Jun 3, 2014

This looks really good. We could probably incorporate the "something missing? find out why" into the "No Result" box as well. Would be good to keep that all in one place.

@oiami
Copy link
Contributor Author

oiami commented Jun 4, 2014

@oalders Do you like the ..."something missing?" part placed before or after the text ? how about before like ..
screenshot 2014-06-04 16 39 48

@oalders
Copy link
Member

oalders commented Jun 4, 2014

@oiami I think that's good. We may want to refine it at some point, but let's see how people react to it first.

@oiami
Copy link
Contributor Author

oiami commented Jun 4, 2014

@tsibley are you going to make the change on my messy regex ? I just pushed another commit (merge 'Something missing ..' into no result box) and seem to be last commit for this pull request if there's nothing else to fix.

);

use Data::Dump qw/dump/;
warn dump( $res->content );
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, these dump lines look like leftovers from debugging :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, this is always my annoying habit that I'm trying to improve. I always too hurry and forget to diff the code before I push :(. I'll fix that.

@rwstauner
Copy link
Contributor

This is some great work.
I'm very glad to see tests, and I like that you moved some markup out to a separate include for reuse.
The screen shots look good too.
Very nice!

@rwstauner
Copy link
Contributor

@oiami TIMTOWTDI, but I would personally change those regexp lines to something like:

my $suggest = $query;
$suggest =~ s/:+/::/g;
if( $suggest ne $query ){

That way the substitution is really simple and the code answers the question
"If I rewrite this to look like a module name, is it different than what they submitted?"

Would that accomplish what you intended?


if ( $results->{total} == 1 ) {
my $module_name
= $results->{results}->[0]->[0]->{module}->[0]->{names};
Copy link
Contributor

Choose a reason for hiding this comment

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

The names key should just be name.

I know this functionality would be difficult to write a reliable test for (since we use live data)
but we might be able to do it by mocking a method in the tests.

I tried searching manually and had success searching for this word: "hashkeyorderbrokenagain".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add test check the response code should be 302 an get new location to the module ('/pod/module::name') is that ok?

@oiami
Copy link
Contributor Author

oiami commented Jun 5, 2014

New changes

  • I really like the solution of regex from @rwstauner, so I cleaned up the code following that
  • Add tests for redirect to 1 result module.
  • Remove debug message
  • Squash a commit

@rwstauner rwstauner merged commit 1701f12 into metacpan:master Jun 9, 2014
@rwstauner
Copy link
Contributor

Looks great! Lots of issues got addressed here.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants