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

Updated bootstrap template #381

Merged
merged 16 commits into from
Sep 30, 2016
Merged

Updated bootstrap template #381

merged 16 commits into from
Sep 30, 2016

Conversation

ralgozino
Copy link
Contributor

Updated bootstrap template emulating the default layout as in "pale_blue" with current upstream master rebase.

@jieter
Copy link
Owner

jieter commented Sep 10, 2016

@ralgozino thanks for the pull, I'll add some line comments.

Also:

  • CI fails, can you look into that?
  • Can you show screenshots of the rendered table.html and the old and new bootstrap.html to this issue? And while your at it, maybe also update/add them in the documentation?
  • I don't like that the bootstrap template needs multiple things added to the class Meta, but the current 'solution' is also not nice. I added a comment to the issue you opened about a solution which is more user friendly in my opinion.
  • I personally don't like having 'page 2 of 4' and '10 of 33 jobs' below my table. I'd vote for removing the latter from both templates. We can only support a very generic use case, any customisation should be done by the developer using the app. Thoughts?

@@ -1,11 +1,12 @@
{% load querystring from django_tables2 %}
{% load i18n %}
{% load l10n %}
Copy link
Owner

Choose a reason for hiding this comment

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

@ralgozino
Copy link
Contributor Author

Thanks for the feedback @jieter

  • I've fixed the line comments and I'm currently looking into the CI failures.
  • I don't like having the totals like '10 o 33 jobs' either; I've removed them from both table.html and bootstrap.html.
  • About the custom class attribute, I will comment on the issue.
  • I Will update the screenshots.

@jieter
Copy link
Owner

jieter commented Sep 18, 2016

Thanks for adding the changes. CI still fails...


<div class="table-container">
{% block table %}
<table class="table table-bordered table-striped"{% if table.attrs %} {{ table.attrs.as_html }}{% endif %}>
<table {% if table.attrs %} {{ table.attrs.as_html }}{%else%}class="table"{% endif %}>
Copy link
Owner

Choose a reason for hiding this comment

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

Please add spaces after and before %.

@@ -55,26 +53,36 @@
{% endblock table %}

{% if table.page and table.paginator.num_pages > 1 %}
{% with table.page.paginator.count as total %}
{% with table.page.object_list|length as count %}
Copy link
Owner

Choose a reason for hiding this comment

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

Aren't these covered by the blocktrans? on line 71?


<div class="ui container table-container">
{% block table %}
<table {% if table.attrs %} {{ table.attrs.as_html }}{%else%}class="ui celled table"{% endif %}>
Copy link
Owner

Choose a reason for hiding this comment

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

Another space issue.

def semantic(request):
'''Demonstrate the use of the semantic template'''
# create some fake data to make sure we need to paginate
if Person.objects.all().count() < 50:
Copy link
Owner

Choose a reason for hiding this comment

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

This block occurs twice now, can you DRY it up?

<h3>django_tables2 with semantic template example</h3>
<div class="ui container">

<!--
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this commented out? Shouldn't you include the stylesheet you added?

.table-container th.desc:after {
content: '\0000a0\0025bc';
float: right;
}
Copy link
Owner

Choose a reason for hiding this comment

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

This is exactly the same as the bootstrap CSS, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the CSS following the bootstrap one to see if it was needed. I've dropped it since is not the case.

@ralgozino
Copy link
Contributor Author

I'm pushing some corrections, but unfortunately I can't make tox work due to a bug with virtualbox and symlinks in shared folders. I'm trying to fix it in order to fix the test that are fialing. Sorry about that.

@ralgozino
Copy link
Contributor Author

@jieter I hope to have addressed all of your comments. Let me know what you think.

Cheers!

@jieter jieter merged commit 2bf8fd3 into jieter:master Sep 30, 2016
@jieter
Copy link
Owner

jieter commented Sep 30, 2016

Finally time to look at this again. Thanks, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants