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

Show URLS for exact redirect #3593

Merged
merged 5 commits into from Apr 18, 2018

Conversation

Projects
None yet
6 participants
@stsewd
Member

stsewd commented Feb 11, 2018

Fix #2431

Now the from and to URLS are shown

screenshot-2018-2-10 edit redirects read the docs

return redirect_text.format(
type=ugettext('Prefix Redirect'),
from_url=self.from_url,
to_url=''

This comment has been minimized.

@humitos

humitos Feb 15, 2018

Member

What about adding something like the docs say?

http://docs.readthedocs.io/en/latest/user-defined-redirects.html

docs.example.com/dev/install.html -> docs.example.com/en/latest/install.html

So, maybe it could say

Prefix Redirect: /dev -> /{default_language}/{default_version}

What do you think?

@humitos

This is a really good improvement! Thanks.

Take a look at my comment and let me know. This should be ready to get merged soon.

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 15, 2018

@humitos done!

screenshot-2018-2-15 edit redirects read the docs

Probably we may want to move this representation to the template in the future.

@humitos

Good job! It's ready to merge for me.

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Feb 20, 2018

Can you post a screenshot of what this looks like with a long redirect path? This will wrap in a strange way.

Similar to some of the other patterns that use this table layout, we should probably move some of the information to a second line of smaller text, similar to the project import page.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Feb 20, 2018

It does wrap somewhat awkwardly. Perhaps putting it on a second line is the best approach.

screen shot 2018-02-20 at 10 18 36 am

On an unrelated note, I cringe a bit anytime I see a button with the label "Submit".

@humitos

This comment has been minimized.

Member

humitos commented Feb 21, 2018

@stsewd would you like to update this PR to use the style you have already used in other PRs (the one with the second line similar to import project page)?

@davidfischer I agree! In this particular case, what would you write? "Create"?

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 21, 2018

@agjohnson @humitos now I think that this styles should be more general, probably is better to do that on #3572 https://github.com/rtfd/readthedocs.org/pull/3572/files#diff-9537de04b5e187580d265c8136166aed and then applied them here.

Meanwhile I'll modify the layout.

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 21, 2018

@agjohnson @humitos this is how it would look like reusing the styles from #3572

(obviously the redirect name would no be on the urls and a icon instead of the remove text)

screenshot-2018-2-21 edit redirects read the docs

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Feb 22, 2018

I think "Create redirect" is great. "Create" is certainly better than "Submit".

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Feb 22, 2018

I think the 2nd line looks good overall.

@humitos

This comment has been minimized.

Member

humitos commented Feb 22, 2018

@stsewd I like your proposal!

I'll use the name of the rediect (Page Redirect, Page Prefix, etc) in the first line and remove it from the second line (gray one). I think it will look nicer.

Seems like everything is converging to a common style. I like that!

@humitos

This comment has been minimized.

Member

humitos commented Mar 23, 2018

@stsewd could you take a look at my last comment? I think that's the only thing missing here to get merged. Can you check if there is anything else missing?

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 23, 2018

@humitos I think is better to reuse the same css classes defined on #3572, the image I show here was just the result of a quick copy/paste to see how it will looks like.

@humitos

This comment has been minimized.

Member

humitos commented Mar 23, 2018

@humitos I think is better to reuse the same css classes defined on #3572, the image I show here was just the result of a quick copy/paste to see how it will looks like.

I refer to this comment:

I'll use the name of the rediect (Page Redirect, Page Prefix, etc) in the first line and remove it from the second line (gray one). I think it will look nicer.

So, applying this to your last example from the screenshot, it would be:

Prefix Redirect
/server/ -> /en/latest/
@stsewd

This comment has been minimized.

Member

stsewd commented Mar 26, 2018

@humitos done, this is how it looks. Didn't like too much, but is better than before.

screenshot-2018-3-26 edit redirects read the docs

@ericholscher ericholscher merged commit d270a67 into rtfd:master Apr 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:redirect-repr branch Apr 18, 2018

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