Skip to content
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

Fix page redirect preview #2711

Closed
wants to merge 1 commit into from
Closed

Fix page redirect preview #2711

wants to merge 1 commit into from

Conversation

@rixx
Copy link
Contributor

@rixx rixx commented Mar 9, 2017

It included '//' - this ought to fix #2380.

It included '//' - this ought to fix #2380.
Copy link
Contributor

@agjohnson agjohnson left a comment

Thanks for the patch! I've noted a cleaner method of implementing this change.

Loading

field_from = field_from_url.val();
field_to = field_to_url.val();
field_from_absolute = (field_from.length > 0) && (field_from[0] === "/");
field_to_absolute = (field_to.length > 0) && (field_to[0] === "/");
Copy link
Contributor

@agjohnson agjohnson Mar 21, 2017

Choose a reason for hiding this comment

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

L27 + L28 are replicating field_to.startsWith() etc

Loading

field_from_absolute = (field_from.length > 0) && (field_from[0] === "/");
field_to_absolute = (field_to.length > 0) && (field_to[0] === "/");
redirect_from = "/$lang/$version" + (field_from_absolute ? "" : "/") + field_from;
redirect_target = "/$lang/$version" + (field_to_absolute ? "" : "/") + field_to;
Copy link
Contributor

@agjohnson agjohnson Mar 21, 2017

Choose a reason for hiding this comment

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

Conditional logic wouldn't be needed here if just using a simple regex to remove leading /. For example:

redirect_from = "/$lang/$version/" + field_from_url.val().replace(/^\/+/, '')

Loading

Copy link
Contributor Author

@rixx rixx Mar 30, 2017

Choose a reason for hiding this comment

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

Oh. yes, that's much cleaner, I'll edit the PR accordingly. Thank you for the review.

Loading

@vidartf
Copy link
Contributor

@vidartf vidartf commented Feb 7, 2018

It would be nice if this could be cleaned up an merged. Threw me off causing me almost half a day's work.

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 11, 2018

@rixx hi, are you still interested on going with this PR? Would be awesome to got your contribution merged.

Loading

@fenilgandhi
Copy link
Contributor

@fenilgandhi fenilgandhi commented Feb 18, 2018

Is there some work here that can be done ? I am looking for a "new-comer" issue but can't find any without a pending PR ! @rixx @stsewd

Update :
Not working on it.

Loading

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Feb 21, 2018

@fenilgandhi That label isn't exclusive! We just tag some issues as easier. If you want to go through the backlog, I'm sure there are more issues you could fix. For now, I think this issue is already covered.

Loading

@vidartf
Copy link
Contributor

@vidartf vidartf commented Feb 22, 2018

Note: As the original author hasn't responded in almost a year, this PR could benefit from:

  • Cloning this branch
  • Adding one or more new commits fixing the review points
  • Resubmitting as a new PR

Loading

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Feb 22, 2018

@vidartf If you'd like to do that, go ahead. :)

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 15, 2018

@vidartf are you still interested on doing the PR? Or maybe @rixx?

Loading

@vidartf
Copy link
Contributor

@vidartf vidartf commented Mar 16, 2018

Loading

@rixx rixx closed this Mar 16, 2018
@rixx rixx deleted the ticket_2380 branch Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants