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

Pinned rows [Feature request / Suggestion] #406

Closed
ctrl-alt-d opened this issue Jan 10, 2017 · 12 comments · Fixed by mozilla/addons-server#4858 or drummonds/bene#50
Closed

Pinned rows [Feature request / Suggestion] #406

ctrl-alt-d opened this issue Jan 10, 2017 · 12 comments · Fixed by mozilla/addons-server#4858 or drummonds/bene#50

Comments

@ctrl-alt-d
Copy link

ctrl-alt-d commented Jan 10, 2017

Would be wonderful to allow pinned rows parameter. Pinned rows will appear always on top. May be something like this:

    creation_date = tables.TemplateColumn(
        template_code = u"""{{record.creation_date|timesince}}""", 
        verbose_name= u"Posted",  
        pinned_by=( '-is_very_important', ),
        order_by=( 'creation_date', 'other_column', )
    )

On rendering, order_by sequence will be pinned_by (unmodified) + order_by (with reverse if needed).

Sorry if this is not the way to suggest improvements.

@ctrl-alt-d ctrl-alt-d changed the title Pinned rows [suggestion] Pinned rows [Feature request / Suggestion] Jan 10, 2017
@djk2
Copy link
Contributor

djk2 commented Jan 20, 2017

It is a good idea.
Could you give more examples how it works?
Please write details, then I try prepare that mechanism.

@jieter
Copy link
Owner

jieter commented Jan 20, 2017

I think this will require a second query to be run in order to have proper performance on big datasets.

@ctrl-alt-d
Copy link
Author

The "Pinned rows" concept means rows that match a criteria are always on the top of results.

You can see this concept on Twitter also on Telegram: https://telegram.org/blog/pin-and-ifttt

For example I would like to pin 'book rows' on my app: https://uf.ctrl-alt-d.net/material/tot/

I think a second query set is not needed, just not to opposite order on checked columns:

order_by=( '*-is_a_book', 'other_column', )

'*' means that is a pinned column, then order is always by '-is_a_book':


    @property
    def opposite(self):
        '''
        Provides the opposite of the current sorting directon.
        Returns:
            `.OrderBy`: object with an opposite sort influence.
        Example:
            >>> order_by = OrderBy('*-name')
            >>> order_by.opposite
            '-name'
        Example:
            >>> order_by = OrderBy('*name')
            >>> order_by.opposite
            'name'

@djk2
Copy link
Contributor

djk2 commented Jan 23, 2017

I think that must work on two data group:

  1. First group - Pinned rows, always rendered on top or bottom of table. Pined rows use list of objects/dicts or queryset.
    get_pinned_data(...) method must return a list of objects/dicts or queryset.
    In template - for each item, value will be get from item by name using last part of accessor.
    A User/developer decide how build data in this method. List can be build using queryset, manually or using other data source.
    I think that should be second method get_pinned_attrs() which return a settings for pined rows like: position: top or bottom, css class and other

Example:

    class Table(tables.Table):
        def get_pinned_data(self, *args, **kwargs):
            return SomeModel.objects.filter(......)
            or 
            return [{'firstname':'John', 'lastname':'Brown', {.....}]

        class Meta:
            model = Simple
  1. Second group is a normal queryset

@djk2
Copy link
Contributor

djk2 commented Jan 29, 2017

jieter, what you think about my proposition?
Can you see better or more flexible solution?

@jieter
Copy link
Owner

jieter commented Jan 30, 2017

This looks like a nice interface to define the table, I have some thoughts/concerns:

  • How to render this in the template without adding to much duplication
  • In this proposal will result in at least extra query.
  • What happens if the return value of get_pinned_data() has a different form?
  • This differs from the proposal of @ctrl-alt-d

@djk2
Copy link
Contributor

djk2 commented Jan 30, 2017

  1. Will prepare html template including in base templates.
    If pinned rows should be displayed on the top then template will be include between tag and {% table.tbody.row %} block,
    If pinned rows should be displayed on bottom then template will be include above tag.

  2. List with data for pinned rows can be a list of objects, list of dictionaries or queryset. The developer decide how build this structure. Returned value from get_pinned_data() method must be iterable. Extra query is not require. If you wont you can add pinned row with summary data or some other information, only you decide.

  3. get_pinned_data method must return structure that is iterating, Single item from that structure is a single pinned row. Value for each column will be getting using column name like: getattr(pinned_row, col_name', table.empty_text). If item/object hasn't attribute the same name as column then will be return empty_value. Maybe if the attribute can't be found should in the attribute then be thrown warning?

  4. I thik, that solution is a more flexible.

I think that the best solution would be if I prepare a proposal branch with code.
After we we will discuss the details
What you think?

sorry for my English

@djk2
Copy link
Contributor

djk2 commented Jan 30, 2017

I started work on this issue.
I reviewed code and I change my proposition.
Changing in templates are not required.
When I finish I push my code to my repository to "pinned_rows"
branche, then you tell what you think ok?

@jieter
Copy link
Owner

jieter commented Jan 30, 2017 via email

djk2 added a commit to djk2/django-tables2 that referenced this issue Jan 31, 2017
@djk2
Copy link
Contributor

djk2 commented Jan 31, 2017

Hi,
could you look on my branch 'pinned-rows-406' with solution for pinned rows?
Maybe you have some suggestions.
If everything is ok, then I prepare pull request

https://github.com/djk2/django-tables2/tree/pinned-rows-406

@jieter
Copy link
Owner

jieter commented Jan 31, 2017

Looks promising, please open the PR, that'll make reviewing the code easy.

djk2 added a commit to djk2/django-tables2 that referenced this issue Mar 1, 2017
jieter pushed a commit that referenced this issue Mar 6, 2017
* Implemented Pinned Rows for issue: #406

* Tests for pinned rows

* Amendments after a code review

* Changes suggested by `felixxm`

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

* Documentation for code by docstrings.

* little fixes

* Documentation for pinned rows

* little fixes in docs

* Fixes after rebase on master
@jieter
Copy link
Owner

jieter commented Mar 6, 2017

Fixed by #406

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment