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
base: master
from

Conversation

Projects
None yet
7 participants
@rixx
Contributor

rixx commented Mar 9, 2017

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

Fix page redirect preview
It included '//' - this ought to fix #2380.
@agjohnson

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

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] === "/");

This comment has been minimized.

@agjohnson

agjohnson Mar 21, 2017

Contributor

L27 + L28 are replicating field_to.startsWith() etc

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;

This comment has been minimized.

@agjohnson

agjohnson Mar 21, 2017

Contributor

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(/^\/+/, '')

This comment has been minimized.

@rixx

rixx Mar 30, 2017

Contributor

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

@vidartf

This comment has been minimized.

Contributor

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.

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 11, 2018

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

@fenilgandhi

This comment has been minimized.

Contributor

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.

@RichardLitt

This comment has been minimized.

Member

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.

@vidartf

This comment has been minimized.

Contributor

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
@RichardLitt

This comment has been minimized.

Member

RichardLitt commented Feb 22, 2018

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

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 15, 2018

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

@vidartf vidartf referenced this pull request Mar 16, 2018

Merged

Fix page redirect preview #3811

@vidartf

This comment has been minimized.

Contributor

vidartf commented Mar 16, 2018

@rixx rixx closed this Mar 16, 2018

@rixx rixx deleted the rixx:ticket_2380 branch Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment