Pinned rows #406 #411

djk2 · 2017-02-01T08:40:11Z

No description provided.

djk2 · 2017-02-04T17:36:33Z

if you looked at my PR?

jieter · 2017-02-04T19:25:22Z

Sorry, I was a bit busy the last couple of days, I'll try to look at it tomorrow.

djk2 · 2017-02-04T21:05:11Z

Ok, no problem.
I wish you a nice weekend :)

jieter

Hi @djk2,

First impression: looks good! I added a couple of comments. I did not have much time, so not really in-depth, but something to work on.

I'll take a more in-depth ook later this week, wednesday/friday night I guess.

jieter · 2017-02-05T21:18:22Z

django_tables2/rows.py

+        """
+        Get raw value from record render
+        this value using by render_func
+        """


Please use single quotes for comments

jieter · 2017-02-05T21:20:14Z

django_tables2/rows.py

+        row_attrs = computed_values(self._table.pinned_row_attrs, self._record)
+        cssClass = "pinned-row"
+        cssClass = " ".join([
+            'even' if next(self._table._counter) % 2 == 0 else 'odd',


This logic seems like it might be cleaner in a separate method (also used by BoundRow.attrs

I created separate method for this code

jieter · 2017-02-05T21:24:01Z

django_tables2/rows.py

+    def generator_pined_row(self, data):
+        """
+        Generator for top and bottom pinned rows
+        """


typo pined should be pinned, and please use single quotes for comments

jieter · 2017-02-05T21:25:14Z

django_tables2/rows.py

        self.data = data
        self.table = table
+        self.top_pinned_data = pinned_data.get('top')
+        self.bottom_pinned_data = pinned_data.get('bottom')


why split here in top/bottom? Does dict().get('top') return None if the key does not exist?

Replaced by one line.
That was only for convenience.
Does not matter.

jieter · 2017-02-05T21:28:07Z

tests/test_pinned_rows.py

+def test_bound_rows_getitem():
+    """
+    Testing __getitem__ from BoundRows
+    Sprawdzenie zwracanej klasy


I don't understand ;)

djk2 · 2017-02-05T21:43:49Z

Great,
In the next few days I will prepare amendments

felixxm

Just few small comments. I didn't have enough time to go through whole PR.

felixxm · 2017-02-07T18:40:12Z

django_tables2/rows.py

@@ -71,12 +71,20 @@ def table(self):
        '''
        return self._table

+    def get_even_odd_css_class(self):
+        '''
+        Return css class `even` or `odd`


Dot is missing: Return css class `even` or `odd`.

felixxm · 2017-02-07T18:51:28Z

django_tables2/rows.py

+        '''
+        Return css class `even` or `odd`
+        '''
+        if next(self._table._counter) % 2 == 0:


Binary & operation instead of % 2may be small optimization (about 25% for 10^6 rows):

if next(self._table._counter) & 1: return 'odd' return 'even'

Not sure what you mean by '25% for 10^6 rows', but 10^6 rows is a very unrealistic scenario: pagination should be used for datasets for more than about 100 rows.

Binary notation is more efficient but is less readable for other people.
PEP 20 -- The Zen of Python:

Simple is better than complex.

Readability counts.

You're right in this scale it's not worth to change it.

felixxm · 2017-02-07T19:28:02Z

django_tables2/rows.py

+            cssClass,
+            row_attrs.get('class', "")
+        ])
+        row_attrs['class'] = cssClass


IMO shorter construction is more readable e.g.:

row_attrs['class'] = " ".join([ self.get_even_odd_css_class(), "pinned-row", row_attrs.get('class', ""), ])

felixxm · 2017-02-07T19:28:33Z

django_tables2/rows.py

+    def attrs(self):
+        '''
+        Return the attributes for a certain pinned row.
+        Add 'pinned-row' to css class attribute


Dot is missing (like above).

felixxm · 2017-02-07T19:29:11Z

django_tables2/rows.py

+        Under the hood this method just makes a call to
+        `.BoundPinnedRow.__getitem__` for each cell.
+        '''
+        for column, value in self.items():


I think that we can iterate over self.values() instead of self.items().

Thanks felixxm for very constructive suggestions. I will try to apply them

felixxm · 2017-02-08T19:25:06Z

django_tables2/rows.py

+        row_attrs['class'] = cssClass
+        return AttributeDict(row_attrs)
+
+    def __iter__(self):


BoundPinnedRow inherits from BoundRow that has exactly the same __iter__ method, hence I think it's unnecessary to replicate it here.

Fact, I didn't see before. Thanks

felixxm · 2017-02-08T19:32:07Z

django_tables2/rows.py

        self.data = data
        self.table = table
+        self.pinned_data = pinned_data or {}


Dictionary is mutable, so maybe it will be better to use:

self.pinned_data = pinned_data.copy() or {}

to avoid changes pinned_data outside __init__ method.

I'm not sure if it correct way.
Both solutions have pros and cons.

felixxm · 2017-02-08T19:33:32Z

django_tables2/rows.py

+
+    def generator_pinned_row(self, data):
+        '''
+        Generator for top and bottom pinned rows


Maybe Top and bottom pinned rows generator. instead of current comment.

felixxm · 2017-02-08T19:38:45Z

django_tables2/tables.py

+        '''
+        Return data for top pinned rows containing data for each row.
+        Iterable type like: queryset, list of dicts, list of objects.
+        Default return None. This method should be overriden in subclass of ~.Table\


overriden -> overridden
~.Table\ ->~.Table

felixxm · 2017-02-08T19:38:54Z

django_tables2/tables.py

+        '''
+        Return data for bottom pinned rows containing data for each row.
+        Iterable type like: queryset, list of dicts, list of objects.
+        Default return None. This method should be overriden in subclass of ~.Table\


overriden -> overridden
~.Table\ ->~.Table

jieter

@djk2 thanks so far, looks good.

I would like to see some explicit documentation for this feature. Can you add a page to the documentation explaining how to achieve pinned rows? Don't be embarrassed by your English skills, we'll progressively improve on the text if needed.

jieter · 2017-02-20T16:14:57Z

django_tables2/tables.py

+        Return data for top pinned rows containing data for each row.
+        Iterable type like: queryset, list of dicts, list of objects.
+        Default return None. This method should be overridden in subclass of ~.Table
+        For None value, top pinned rows are not rendered.


Did you check the output for this in the documentation? I'd like to see backticks around None to make it appear like code.

Please also use the more specific docstring syntax for the return value, to allow sphinx to render it like a return value. For example:

returns: `None` (default) no pinned rows at the top, iterable, data for pinned rows at the top.

Ok, I try prepare some documentation for this feature,
I also make fixes formatting docstring.

jieter · 2017-02-20T16:18:44Z

tests/test_rows.py

+    count = 0
+
+    for row in simple_table.rows:
+        css_class = 'even' if count % 2 == 0 else 'odd'


I'd say repeating the logic in a test is not the way to go. also: this test tests some different things as well. Maybe just check that the next value differs from the previous?

You're right, your proposal is definitely better.

jieter · 2017-02-20T16:19:31Z

django_tables2/rows.py

+        '''
+        if next(self._table._counter) % 2 == 0:
+            return 'even'
+        return 'odd'


I think this can be condensed to a one-liner.

No problem.

jieter · 2017-02-20T16:23:31Z

django_tables2/rows.py

@@ -71,12 +71,20 @@ def table(self):
        '''
        return self._table

+    def get_even_odd_css_class(self):
+        '''
+        Return css class `even` or `odd`.


maybe the word alternatingly or something like that should be added to this docstring?

jieter

@djk2 thanks, looks good. I commented on some typo's.

Did you verify the rendered version of the docstrings using tox -e docs and inspecting the resulting html?

jieter · 2017-02-27T09:02:23Z

docs/pages/pinned-rows.rst

+
+Pinned rows attributes
+========================
+If you wont override HTML attributes for pinned rows you can use: ``pinned_row_attrs``.


wont -> won't

wont -> want

Ah, I was not paying enough attention:

I think it should be "If you want to override..."

jieter · 2017-02-27T09:03:23Z

docs/pages/pinned-rows.rst

+Pinned rows attributes
+========================
+If you wont override HTML attributes for pinned rows you can use: ``pinned_row_attrs``.
+Pined row attributes can be specified using a `dict` defining the HTML attributes for


Pined -> Pinned

jieter · 2017-02-27T09:03:45Z

docs/pages/pinned-rows.rst

+===========
+
+By using Pinned Rows, you can pin particular rows to the top or bottom of your table.
+To add pinned rows to your table, you must overridden `get_top_pinned_data` and/or `get_bottom_pinned_data`


overridden -> override

djk2 · 2017-02-27T09:29:34Z

I ran tests for documentation using tox and checking the results in the browser.
I generate documentation using the command make html

jieter · 2017-03-01T15:16:41Z

Hmm, apart from the 'want' -> 'want to' which I commented on, I see nothing preventing me to merge this.

Unfortunately, some of my work on master makes that github cannot automatically merge this. Can you rebase your branch on current master?

jieter · 2017-03-06T10:23:54Z

@djk2 Merged, thanks!

jieter · 2017-03-06T12:53:12Z

released 1.4.2

djk2 · 2017-03-06T14:36:38Z

I'm honored. Thank for cooperation.

jieter reviewed Feb 5, 2017

View reviewed changes

felixxm reviewed Feb 7, 2017

View reviewed changes

felixxm reviewed Feb 8, 2017

View reviewed changes

jieter reviewed Feb 20, 2017

View reviewed changes

djk2 force-pushed the pinned-rows-406 branch from 23dc84c to b3568e2 Compare February 26, 2017 20:16

jieter reviewed Feb 27, 2017

View reviewed changes

djk2 added 10 commits March 1, 2017 20:07

Implemented Pinned Rows for issue: jieter#406

a655d63

Tests for pinned rows

a155204

Amendments after a code review

4c6ccff

Changes suggested by felixxm

2c2503e

Test - ValueError exception when data for pinned rows are not iterable

3a7f6bf

Documentation for code by docstrings.

41d9149

little fixes

cf918b6

Documentation for pinned rows

aa847fd

little fixes in docs

c0cfaca

Fixes after rebase on master

5c62d77

djk2 force-pushed the pinned-rows-406 branch from a8af300 to 5c62d77 Compare March 1, 2017 19:26

jieter merged commit ccc1d9a into jieter:master Mar 6, 2017

pyup-bot mentioned this pull request Mar 6, 2017

Update django-tables2 to 1.4.2 mozilla/addons-server#4858

Merged

pyup-bot mentioned this pull request Oct 2, 2017

Pin django-tables2 to latest version 1.11.0 drummonds/bene#17

Closed

pyup-bot mentioned this pull request Oct 10, 2017

Pin django-tables2 to latest version 1.12.0 drummonds/bene#27

Closed

This was referenced Oct 12, 2017

Initial Update mwolff44/pk-sbc#20

Closed

Pin django-tables2 to latest version 1.13.0 drummonds/bene#32

Closed

This was referenced Nov 18, 2017

Update django-tables2 to 1.14.2 mwolff44/pk-sbc#31

Closed

Update django-tables2 to 1.15.0 mwolff44/pk-sbc#61

Closed

pyup-bot mentioned this pull request Nov 27, 2017

Update django-tables2 to 1.16.0 mwolff44/pk-sbc#64

Closed

Pinned rows #406 #411

Pinned rows #406 #411

Conversation

djk2 commented Feb 1, 2017

djk2 commented Feb 4, 2017

jieter commented Feb 4, 2017

djk2 commented Feb 4, 2017

jieter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djk2 commented Feb 5, 2017 • edited Loading

felixxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixxm Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djk2 Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djk2 Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djk2 commented Feb 27, 2017 • edited Loading

jieter commented Mar 1, 2017

jieter commented Mar 6, 2017

jieter commented Mar 6, 2017

djk2 commented Mar 6, 2017

djk2 commented Feb 5, 2017 •

edited

Loading

felixxm Feb 7, 2017 •

edited

Loading

djk2 Feb 8, 2017 •

edited

Loading

djk2 Feb 20, 2017 •

edited

Loading

djk2 commented Feb 27, 2017 •

edited

Loading