My path looks like http://localhost/core/#overlay=admin%2Fconfig%2Fdevelopment%2Fcoder%2Fup...

I would love it to look like http://localhost/core/#overlay=admin/config/development/coder/upgrade instead, for easier copy/paste of exact paths to aid in reporting bugs.

I have no idea if this is possible, but would love if it was. :) If not, maybe convert the separator to -s instead so it's not so oooglay?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

casey’s picture

Is a '/' after a '#' valid or should it be encoded?

casey’s picture

Status: Active » Needs review
FileSize
818 bytes
ksenzee’s picture

Great! This is another obnoxious little thing about the overlay that I think is subconsciously annoying the heck out of some developers.

I would recommend assigning the return value to a variable, then returning that variable. Easier to read, especially for the poor jQuery newbies otherwise known as Drupal core developers.

+++ modules/overlay/overlay-parent.js	24 Dec 2009 00:30:53 -0000
@@ -778,7 +778,12 @@
+  .replace(/(#overlay=)(.+)(\?|#|$)/, function(str, prefix, link, suffix) {

I don't know if we can guarantee that "overlay" will always be the first parameter in the fragment string, especially since BBQ is in core now.

This review is powered by Dreditor.

casey’s picture

FileSize
844 bytes

You're right. Makes it a little more complicated. Do I need to explain in detail what the regexp is all about?

ksenzee’s picture

Whew. It certainly did get more complicated. Some explanation wouldn't be a bad idea.

Also there's some stray whitespace on the blank line.

casey’s picture

FileSize
948 bytes

Small comment added what the regexp does + minor improvement of regexp.

Assing to a variable and then return that variable seems nonsense to me. But feel free to change this.

Re-test of from comment #2404430 was requested by @user.

ksenzee’s picture

FileSize
817 bytes

Avoiding a long chained return value is important primarily because it promotes readability, especially for those who are new to jQuery, which (as I said a bit flippantly above) is a lot of Drupal developers. I'd personally like to see all multi-line return statements officially frowned upon in the coding standards. But in this case the long return value is especially unreadable, because there's a replace() statement with an anonymous inner function that also returns a value.

casey’s picture

You can drop some braces:

var newLink = ($.param.fragment(base, {'overlay': destination}));
var newLink = $.param.fragment(base, {'overlay': destination});

string.replace() is plain old javascript, no jQuery.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

casey’s picture

Status: Needs work » Needs review
FileSize
943 bytes

Status: Needs review » Needs work

The last submitted patch, , failed testing.

Status: Needs work » Needs review

Re-test of from comment #2405510 was requested by @user.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

webchick’s picture

I'm trying really hard to figure out how a patch that changes some JS is causing filefields to not validate.

Kiphaas7’s picture

why not just use escape()/unescape()? Granted, it has a few issues, but for the case here (prettify urls) it shouldn't be harmfull...

http://cass-hacks.com/articles/discussion/js_url_encode_decode/

casey’s picture

I agree we could use unescape but only on the #overlay={url} part. So we still need that regexp. Escaping everything could potentionally break other modules. I think we should not risk that.

casey’s picture

FileSize
901 bytes

Ok much simpler approach.

casey’s picture

Status: Needs work » Needs review
nvanhove’s picture

Status: Needs review » Needs work

Tested on IE6, IE8as7, IE8, Chrome.
This patch generally works, but not completely.
Normal URLS work okay:
drupal/node#overlay=admin/people

But when there are arguments, it isn't okay:
drupal/user/1/edit?destination=admin/people%3Frender%3Doverlay

casey’s picture

Status: Needs work » Needs review
FileSize
939 bytes

querystring in fragment can't be replaced jquery-bbq can't handle that.

This is the best thing we can get when using jquery-bbq (storing multiple urls in the url fragment).

casey’s picture

FileSize
901 bytes

Oops wrong patch.

ben_alman’s picture

If you're looking for a general solution to cleaning specific encoded params from a hash (generated by BBQ for example), something like this will work. Just beware that while most browsers encode params in their object-property-declaration order, that order is not guaranteed, so if there are multiple params, "overlay=" might not always follow "#" (even if overlay is the first property in the params object).

function clean_hash( url ) {
  var parts = url.split('#');
  if ( parts[1] ) {
    // decode %2F, %5B, %5D (/[])
    parts[1] = parts[1].replace( /%(?:2F|5B|5D)/gi, decodeURIComponent );
  }
  return parts.join('#');
}

var url = $.param.fragment( '/test.html?a/b/c', { x:1, y:'/foo/bar[baz]/!@#', z:2 } );

url;                               // "/test.html?a/b/c#x=1&y=%2Ffoo%2Fbar%5Bbaz%5D%2F!%40%23&z=2"
clean_hash( '/test.html?a/b/c' );  // "/test.html?a/b/c"
clean_hash( '/test.html?a/b/c#' ); // "/test.html?a/b/c#"
clean_hash( url );                 // "/test.html?a/b/c#x=1&y=/foo/bar[baz]/!%40%23&z=2"
casey’s picture

FileSize
896 bytes
+  var regexp = new RegExp("(?:[#&]overlay=)" + escape(path).replace(/\//g, '%2F'));

It contains a character set so it matches either #overlay= and &overlay=

decodeURIComponent is however very nice.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

It works for me. (Reading through it was fair too.) I can't see any holdup on it.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

OMG IT'S CHRISTMAS!!!!!

Thank you so much. :) Committed to HEAD!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.