Code review comment for lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting

Revision history for this message
John A Meinel (jameinel) wrote :

I guess my specific point is, people expect inline controls like a triangle
to be cheap, but at this point to do it accurately we have to reload the
page. Should we present this in a way to make it clearer what will happen?
Would Ajax be enough to make the reload lightweight?. We should probably
test with a large dataset and see how long it takes to reload.

John
=:->
On Oct 20, 2012 7:52 AM, "John A Meinel" <email address hidden> wrote:

> I haven't gone through this in detail, but my first concern is how this
> interacts with pagination. Does this just sort the visible nodes, or does
> it re do the query with a new sort order? (It is nice to nit have to reload
> the page, but with >1k nodes, we won't display them all at once, and then
> sorting wouldn't be very useful)
>
> John
> =:->
> On Oct 19, 2012 8:31 PM, "Dimiter Naydenov" <
> <email address hidden>>
> wrote:
>
> > The proposal to merge
> > lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting into
> > lp:maas has been updated.
> >
> > Description changed to:
> >
> > Implemented sorting for /MAAS/nodes/ list - by hostname or status
> > (asc/desc). As advised, I pulled only what was needed from the project
> > django-sortable and I'm using it in the views/templates. Since
> > django-sortable is not in main archive, we cannot use it, but it's only a
> > tiny package, so I extracted what was needed and added it to maasserver
> > (also did some style formatting so it'll conform to our code).
> >
> > There are a couple of very minor UI changes, which I was advised also to
> > request a separate review from Huw Wilkins. The added elements are 2
> > up/down arrows appearing on the right of the two table columns - MAC and
> > Status. Here are screenshots how it looks in all states:
> >
> > http://people.canonical.com/~dimitern/maas-nodes-sort-none.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-mac-asc.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-mac-desc.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-status-asc.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-status-desc.png
> >
> > For more details, see:
> >
> >
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> > --
> >
> >
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> > You are subscribed to branch lp:maas.
> >
>
> --
>
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> You are subscribed to branch lp:maas.
>

« Back to merge proposal