Editable Reblogs: redirect after reblogging from a blog page

[bvtsang]

Fixes #790

[bvtsang]

If #828 gets merged before this pull request, then this version bump will need to be updated; vice-versa if this gets merged first.

[nightpool]

IMO we should get into the habit of making pull requests against new-xkit
branches, so we can do stuff like collaborate on them. (like would be
useful with this and #828)

thoughts?

On Fri, Dec 4, 2015 at 3:20 AM Bryan Tsang notifications@github.com wrote:

If #828 #828 gets merged before
this pull request, then this version bump will need to be updated;
vice-versa if this gets merged first.

—
Reply to this email directly or view it on GitHub
#832 (comment).

[Wolvan]

@nightpool that should be discussed in another issue. (Also, I honestly don't fully understand, what do you mean?)

[bvtsang]

@Wolvan I think he's referring to a case where people are working on different issues for the same extension. Right now, @blackjackkent and I are both working on fixes for Editable Reblogs: she's working on #828 and I'm working on this pull request. The problem is that if @blackjackkent's pull request gets merged, then I'll need to update my version of Editable Reblogs in this pull request; she will need to do the same if this gets merged first. I'm sure you're familiar with endless rebasing hell with version changes, so we don't need to get into why this is annoying.

Correct me if I'm wrong, but what @nightpool is suggesting is that we can avoid this problem by working on the same branch. For example, if @blackjackkent pushed her changes for #828 to an editable-reblogs branch in this new-xkit/XKit repository, then I could simply pull that branch and add my changes to it without either one of us fussing over the version number.

Since this is a problem that goes beyond this pull request, I agree that this should ultimately be discussed in a new issue.

[nightpool]

this only works if "redirect_to" is the only search parameter. prefer using the jquery param method

[bvtsang]

I get this doesn't handle the case where redirect_to isn't the only search parameter, but can you help me understand how $.param helps? It takes an array or an object and returns its serialized representation, but we want is the reverse: to deserialize the query parameter and redirect to it.

The solution I'm thinking of will probably be something like this.

[nightpool]

I wasn't referring specifically to param, but just that I'm pretty sure
there's a jquery function to do it

On Sat, Dec 5, 2015 at 8:25 PM Bryan Tsang notifications@github.com wrote:

In Extensions/editable_reblogs.js
#832 (comment):

@@ -358,6 +358,13 @@ XKit.extensions.editable_reblogs = new Object({
XKit.interface.kitty.set(response.getResponseHeader("X-tumblr-kittens"));
XKit.tools.add_function(function() {
Tumblr.Events.trigger("postForms:saved");

•                   var query_string = window.location.search;
  

•                   if (query_string.indexOf("redirect_to=") !== -1) {
  

•                       var redirect_url = window.location.search.split("redirect_to=")[1];
  

I get this doesn't handle the case where redirect_to isn't the only
search parameter, but can you help me understand how $.param
https://api.jquery.com/jquery.param helps? It takes an array or an
object and returns its serialized representation, but we want is the
reverse: to deserialize the query parameter and redirect to it.

The solution I'm thinking of will probably be something like this
http://stackoverflow.com/a/901144/2073440.

—
Reply to this email directly or view it on GitHub
https://github.com/new-xkit/XKit/pull/832/files#r46765394.

[nightpool]

looks like I was wrong—there are a lot of libraries that do it, but no
standard way. I'd prefer a helper function in XKit.tools for the
implementation if that works for you

On Sat, Dec 5, 2015 at 8:30 PM Evan G eg1290@gmail.com wrote:

I wasn't referring specifically to param, but just that I'm pretty sure
there's a jquery function to do it

On Sat, Dec 5, 2015 at 8:25 PM Bryan Tsang notifications@github.com
wrote:

In Extensions/editable_reblogs.js
#832 (comment):

@@ -358,6 +358,13 @@ XKit.extensions.editable_reblogs = new Object({
XKit.interface.kitty.set(response.getResponseHeader("X-tumblr-kittens"));
XKit.tools.add_function(function() {
Tumblr.Events.trigger("postForms:saved");

•                  var query_string = window.location.search;
  

•                  if (query_string.indexOf("redirect_to=") !== -1) {
  

•                      var redirect_url = window.location.search.split("redirect_to=")[1];
  

I get this doesn't handle the case where redirect_to isn't the only
search parameter, but can you help me understand how $.param
https://api.jquery.com/jquery.param helps? It takes an array or an
object and returns its serialized representation, but we want is the
reverse: to deserialize the query parameter and redirect to it.

The solution I'm thinking of will probably be something like this
http://stackoverflow.com/a/901144/2073440.

—
Reply to this email directly or view it on GitHub
https://github.com/new-xkit/XKit/pull/832/files#r46765394.

[Wolvan]

I have used code to get GET args multiple times, gonna implement that in XKit.tools later

[nightpool]

reviewed. params needs a fix, otherwise looks fine.

[bvtsang]

@nightpool Added a helper function in XKit.tools to get query parameters.

[nightpool]

confused about what the backslashes in this character class are for. mind explaining?

[bvtsang]

It escapes the first set of brackets so that it meets the URI format listed in RFC 3986. For example, it will convert url[123] into url\[123\].

But on closer inspection:

• it escapes only the first set of brackets, so a string like url[123][456] would turn into url\[123\][456]
• it escapes via backslashes, not percent encoding
• as you mentioned, in this particular case the string is already encoded so chances are it won't find the bracket anyway

(Note to self: don't blindly copy functions from StackOverflow. Note to other developers: don't be me.)

Assuming XKit.tools.getParameterByName will be a util function for generic use, we have the following options:

• accept this as is
• find an alternative that percent-encodes brackets
• depending on how much we expect this case, find a solution that handles getting the parameter x from a URL like http://www.mysite.com/index.php?x=x1&x=x2&x=x3
  • considering this deeply may bring us down the rabbit hole of creating bulletproof util functions, or functions that handle every case thrown at them
  • depends on what we think is "good enough"
• something else

What do you suggest?

[nightpool]

good enough for me would be just replace line 228 with name = encodeURIComponent(name)

EDIT: whoops

[nightpool]

hmm. only qualm is that it doesn't URIencode before searching for the name, so in most browses (I think) line 228 won't have any effect. (because it's searching for [whatever], but the URI will look like %5Bwhatever%5D)

[nightpool]

we're decoding URI component twice. (once here and once on line 373 of ER)

also, a nit, but I think the ternary, function call, array index, and replace method is a little too much complexity for one line and would prefer to see it separated.

[nightpool]

@homu r+

[homu]

📌 Commit d3fa653 has been approved by nightpool

[homu]

⚡ Test exempted - status