-
Notifications
You must be signed in to change notification settings - Fork 426
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 #499 - add default "class" HTML attribute to footers #501
Conversation
…ause of more and more complicated HTML generated by dt2
…l be analysing HTML and not just comparing simple strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpasternak thanks for working on this. I added some notes/comments, can you have a look?
@@ -43,7 +43,7 @@ | |||
{% if table.has_footer %} | |||
<tr> | |||
{% for column in table.columns %} | |||
<td>{{ column.footer }}</td> | |||
<td {{ column.attrs.tf.as_html }}>{{ column.footer }}</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double space here
A ``th``, ``td`` and ``tf`` are guaranteed to be defined (irrespective | ||
of what's actually defined in the column attrs. This makes writing | ||
templates easier. ``tf`` is not actually a HTML tag, but this key name | ||
will be used for attributes for column's footer, if the column has one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the tf
key here. Maybe footer
is better, although that is a html tag, but not relevant to a table...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have given it a lot of thought and chosen the best abbreviation, in my opinion, having in mind the tags and the actual non-existence of "the footer tag". I also understand why you disagree because it looks like I've been there too.
Please let me know what key name will you decide and I'll gladly update the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's go for tf
for now, thanks for explaining
docs/pages/custom-rendering.rst
Outdated
</tr> | ||
|
||
|
||
You can also specify ``attrs`` attribute when creating a column. django-tables2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a relevant notion but maybe better to link to :ref:column-attributes
and maybe adjust the documentation over there?
Also note I just added a fix for #241, so make sure to rebase on master before attempting to edit the docs there.
tests/test_footer.py
Outdated
soup = BeautifulSoup(html, "lxml") | ||
row = soup.find("tfoot").tr | ||
columns = row.find_all("td") | ||
assert "align" in columns[1].attrs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having BeautifulSoup in the tests is nice, but does it allow a little more concise syntax? It would be nice to have some convenient helper in test/utils.py
. It would also be nice to ditch the bare lxml-related stuff in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not being a native English speaker, I am not sure if I understood you well enough. What do you mean by "concise"? Less lines? I have compared the other option which was something along assert "align" in soup.find("tfoot").tr.find_all("td")[1].attrs
and it was kind of unreadable to me. I also went with the more sparse syntax so you can actually easily plug in new people into the project and understand, what is actually happening. From my experience, you can be really concise with CSS selectors but the thing quickly becomes unreadable. So I have chosen the best thing I could come up with. If you have any other suggestion how should I re-write this code, please let me know, as I currently don't have other ideas. Maybe that's just a matter of taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm no native either, so double distorted communication... please feel free to ask for clarification... And indeed, my comment isn't totally clear.
I meant to address that 'we' already have some mechanism in place to do what you're doing with BeautifulSoup with lxml, for example:
django-tables2/tests/columns/test_general.py
Line 273 in 26f2fc2
assert root.findall('.//thead/tr/th')[0].attrib == {'key': 'value', 'class': 'a orderable'} |
I'm not against adopting a new way to do it, but I prefer using one way across the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it now thanks for explanation. I’ll try rewriting that with lxml if it is a dependency already.
EDIT: done, pushing my branch in a moment. My bad for not checking if something like bs4 is not being used already.
Hi, I've modified the files as per your comments. I'm not exactly sure if I understood you properly when you wrote about rebasing. I merged the upstream in fb4ed23 . I'm not sure if I have enough git experience, so I'm not sure if that was proper. If I did that wrong, please give me some hints if possible. The last commit before pulling upstream was 0500b76 |
@mpasternak merged, thanks. Looking forward to your next contribution! There were some things I fixed after merging, most notably you omitted the change to templates/bootstrap.html. Should be all fixed now. |
Hi,
this is a fix for #499. It adds a default
class
attribute to footers in a way, that can be styled and overridden byColumn.attrs['tf']
.It is documented in docs and in
CHANGELOG.md
.On my machine,
tox
was green before creating this PR.Let's discuss, review and eventually merge, then I'll work on the actual content of the class attribute as per #497 .