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

Show URLS for exact redirect #3593

Merged
merged 5 commits into from
Apr 18, 2018
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 11, 2018

Fix #2431

Now the from and to URLS are shown

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

@RichardLitt RichardLitt added Improvement Minor improvement to code PR: ready for review labels Feb 14, 2018
return redirect_text.format(
type=ugettext('Prefix Redirect'),
from_url=self.from_url,
to_url=''
Copy link
Member

@humitos humitos Feb 15, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

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

@agjohnson
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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

@davidfischer
Copy link
Contributor

I think the 2nd line looks good overall.

@humitos
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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 readthedocs:master Apr 18, 2018
@stsewd stsewd deleted the redirect-repr branch April 18, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants