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

Better version listing for project dashboard #3673

Closed

Conversation

shubham76
Copy link
Contributor

@shubham76 shubham76 commented Feb 24, 2018

This PR is regarding Issue #3554.
First 4 out of 5 requirements.
Please correct me if I made any mistakes.

@shubham76 shubham76 changed the title Better version listing for project dashboard - Issue: #3554 Better version listing for project dashboard Feb 24, 2018
The file was changed by mistake. Restored it.
@@ -91,6 +91,15 @@ <h3>{% trans "Project Privacy Level" %}</h3>
</div>
{% endblock %}

{% block project-default-branch %}
<h3>{% trans "Project Default Branch" %}{# trans does not work here. #}</h3>
Copy link
Member

Choose a reason for hiding this comment

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

Why that comment {# trans does not work here. #}?

Copy link
Contributor Author

@shubham76 shubham76 Feb 24, 2018

Choose a reason for hiding this comment

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

{% trans "Project Default Branch" %}
The language for this should change when we change the website language. But somehow it doesn't know the right word for 'Default' in other languages and therefore it stays in English language only. I couldn't figure out why that happens. That's why I wrote the comment.
Should I remove the comment?

Copy link
Member

Choose a reason for hiding this comment

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

that's because of missing translations for that word, RTD uses transifex for collaborating on i18n https://docs.readthedocs.io/en/latest/i18n.html. It's safe to remove that comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. Okay! I will remove the comment.

@@ -19,40 +19,42 @@
<h3>{% trans "Versions" %}</h3>
</div>

<div class="module-list">
Copy link
Member

Choose a reason for hiding this comment

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

I think this div is still needed, can you post a screenshot of how it looks?

Copy link
Contributor Author

@shubham76 shubham76 Feb 25, 2018

Choose a reason for hiding this comment

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

tests-demo _ read the docs
Because of removing that div it looks like the version page listing.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, rtd is moving to match the previous style on all lists (see #3445 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the issue #3554 here says it should match the style on project version listing page.
What should be done?

Copy link
Member

@stsewd stsewd Feb 25, 2018

Choose a reason for hiding this comment

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

I'm not sure, lets wait to agjohnson's reply, but on other issue this was already discussed (see #3572, relevant comments: #3572 (comment), #3572 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I saw that. I still went ahead with this issue as there were other improvements as well.
I hope that's fine. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The styling that was removed here was indeed the correct styling, it should be added back. The styling wasn't the issue, but the table content. We're also missing an opportunity to have a single template for 3 different pages, so that is the larger focus here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! In that case, the previous design should remain same. Got it! I'll change the styling back again and keep the table content modified as it is in PR.

{# Link to the docs #}
<a class="module-item-title" href="{{ version.get_absolute_url }}">
{{ version.slug }}
{% if request.user in project.users.all %}
Copy link
Member

Choose a reason for hiding this comment

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

You can use {% if request.user|is_admin:project %} for checking for the right user. You will need to load this tag {% load privacy_tags %} (if itsn't already loaded)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Right. I'll make the changes straight away.

@@ -91,6 +91,15 @@ <h3>{% trans "Project Privacy Level" %}</h3>
</div>
{% endblock %}

{% block project-default-branch %}
<h3>{% trans "Project Default Branch" %}</h3>
Copy link
Member

Choose a reason for hiding this comment

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

From #3554

The front dashboard page version listing should note the default branch

I think this info should go on the list, not on the side bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh Yes. I thought so. But I wasn't clear about the requirement.
It is there on the list as shown in the picture.
Should I remove these lines then?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on default version noted in the table, not the sidebar.

@agjohnson
Copy link
Contributor

The other piece to this work is to make the version lists a single template, so that we aren't duplicating patterns in two different places. There could be some element that don't fit, and perhaps that can be handled conditionally through a variable passed to the template include.

@agjohnson
Copy link
Contributor

Also, this will conflict with #3445, which fixes some of the style level problems with the version page.

@RichardLitt RichardLitt added the PR: work in progress Pull request is not ready for full review label Mar 2, 2018
@shubham76
Copy link
Contributor Author

Thanks for the review @agjohnson!

The other piece to this work is to make the version lists a single template, so that we aren't duplicating patterns in two different places. There could be some element that don't fit, and perhaps that can be handled conditionally through a variable passed to the template include.

Okay. So we need a single Django-template to insert into project_details.html & project_version_list.html so we don't have to repeat the code. Is that correct?

Also, this will conflict with #3445, which fixes some of the style level problems with the version page.

Oh. Actually, I am new to the open source. What should be done in that case?

@stsewd
Copy link
Member

stsewd commented Mar 2, 2018

What should be done in that case?

@shubham76 if your changes cross to much, I think the best is to wait to #3445 to gets merged. If not, feel free to continue!

@shubham76
Copy link
Contributor Author

@stsewd Okay. Thanks for replying! :)

@@ -18,9 +18,9 @@
<div class="module-header">
<h3>{% trans "Versions" %}</h3>
</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove Extra spaces !

Copy link
Contributor Author

@shubham76 shubham76 Mar 4, 2018

Choose a reason for hiding this comment

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

Well, it has been like that in the original code. https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/templates/core/project_details.html. I did not try to change that style.

Copy link
Contributor

@bansalnitish bansalnitish Mar 4, 2018

Choose a reason for hiding this comment

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

The line is fine. I am referring to the extra whitespace you have added. It was not in the original code.

<div class="module-list">

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto !

@humitos
Copy link
Member

humitos commented Oct 10, 2018

Hi all! What's the status of this PR?

I think it was "kind of blocked" on #3445 which was already merged. So, I suppose that you can rebase it and use the styles that PR introduces.

@ericholscher
Copy link
Member

Closing this, as it hasn't been updated in a few months. Feel free to reopen with requested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants