aristippe / pathagar

Pathagar is a simple bookserver serving OPDS feeds
GNU General Public License v2.0
1 stars 1 forks source link

Switch to django-pure-pagination #35

Closed aristippe closed 8 years ago

aristippe commented 8 years ago

django-pure-pagination looks to be the most maintained of any of the available paginators, and the best thing is it requires no code changes. I liked the Bootstrap pagination theme the most, so I used their builder to make a CSS file with only pagination and then stripped out the few remaining unrelated styles.

Have a look. I think with this we can close #33. Looks good to me and no effort needed to create our own code.

capture d ecran 2016-02-12 a 22 56 15

The repo if you'd like to take a look:

https://github.com/jamespacileo/django-pure-pagination

aristippe commented 8 years ago

btw, there's one small issue. I added showing file size to book_detail (book.book_file.size|filesizeformat). Since now that we can link files, there's the possibility that book_file might not exist and that will throw an error. It would be nice to show size; would the best way be to add something to context like file_exists?

There's also a number of comments column in book_list. Since commenting isn't always used, I'll add a conditional to only show it if enabled.

sinergatis commented 8 years ago

I like the spirit of this pull request, but I'm a bit hesitant about the solution (probably my overly-zealous and preemptively-lazy side taking over!). It basically boils down to:

In this particular scenario, if you like Bootstrap pagination style, and if we decide on using a proper template framework, and if that framework is Bootstrap, and if the chosen package is django-bootstrap3 ... we probably would have this pagination issue solved "for free" by the use of the pages_to_show parameter of the boostrap_pagination template!

In short: I'm not opposed to using django-pure-pagination (looks OK and safe to use) or to this pull request, and personally don't place much weight on the fronted and fully trust your criteria in that regard, but if in the end is a temporary fix I feel deviating a bit and working on #18 first would be a better approach!

sinergatis commented 8 years ago

Since now that we can link files, there's the possibility that book_file might not exist and that will throw an error. It would be nice to show size; would the best way be to add something to context like file_exists?

Hmmm, probably just enclosing within something similar to {% if file %} would do the trick, in order to try to avoid passing extra cruft in the context?

aristippe commented 8 years ago

I understand and agree with the point of carefully evaluating external libraries in regards to how well they are maintained and considering if they may break with a future Django upgrade. I have been trying to keep that in mind while looking at other possible libraries. I did look at other pagination packages and I believe most of them haven't been updated in a while, and while I didn't try the others, they didn't at first glance provide exactly the functions I was looking for. I didn't have an exact spec at first, though looking over how each of them behaved, I ended up liking pure-pagination the most.

I don't consider django-pure-pagination a temporary fix; for the moment, I like how it functions: the outside buttons act as previous/next, it shows first and last page numbers while allowing customizable number of pages to include next to first and last (1 in the screenshot above, <1…2345…100>). It looks like pure-pagination is well maintained, though that could change in the future. If that happened, it might not be so bad to fork and fix as needed. Switching to another package in the future might not be so bad either since it affects only a small part of the code. I took a quick look at django-bootstrap3 and the pagination acts a bit differently (arrows act as first/last, doesn't show first/last page numbers).

capture d ecran 2016-02-14 a 13 13 45

On the question of Bootstrap, while I like some of it's functions, such as the ease of creating responsive sites for mobile, and I was considering possible future template changes like allowing user options for showing book_list as a grid cover (with title, etc below, similar to an image gallery), or some other list view that was less tabular but showed other details like a truncated description, overall I'm not too fond of some aspects of Bootstrap, like the look of some elements such as headers and buttons. Perhaps that can be customized, though evaluating and switching to an entire framework and then customizing it as necessary looks like a fairly large project. I need to examine to what extent Bootstrap is customizable and how many have been able to create sites that use Bootstrap or another framework while not looking like they use it. I have been thinking about how to change some elements, such as the header and toolbar, though I'm not yet sure what they could look like.

I am also hesitant to bring in the whole of one framework; it changes the look of every element on every page, and would require a good amount of fixing, either to revert to the old look, or adapt the template code to the framework. While we are now depending on the original framework included, blueprint, I did want to eventually change that though I'm not yet sure to which. If another framework is used, I'd prefer to evaluate it per element, toolbar, buttons, menus, etc., and deciding which framework provides the nicest implementation and bringing in each element individually as needed.

So overall, I do quite like django-pure-pagination, it seems well maintained, and I like it most so far in terms of its implementation. For using other frameworks, I'd prefer to examine them per element instead of redesigning each element of every template to adapt to an entire framework. While it could eventually be the case that we use an entire framework for every page element, it requires a good amount of work to adapt the templates as well as customizing it so it doesn't look exactly like any other site.

sinergatis commented 8 years ago

Having read your arguments against Bootstrap (or any other large, well-established framework - the same reasoning will probably apply), and having worked with Django applications that use "wild, pure CSS+HTML" and others that use a CSS-framework layer of sorts I can't say I'm convinced that the benefit outweigh the possible drawbacks you mention!

The visual style and looks of specific elements it's basically a product of its design, and really easy to customize (and meant to be customized right from the start) by using some tiny CSS on top of the defaults, etc. After all, the important word here is framework: having a sane, modular, sound, "standard" and reusable set of components and guidelines that streamlines the development of templates and provides a bunch of goodies (flow, layout, usability, etc) for "free", regardless of the specific visual adjustments, is a really huge plus that it's really hard to ignore. Granted, it would take some adjusting, for sure, but considering that there has been ~1000 changes in static and templates since the initial fork, and that some existing templates and CSS are still in need of a cleanup anyway, I don't really expect the adjusting to be such a big task and actually should require less effort than keeping hacking away at the templates and the CSS (and would go a long way for reducing the amount of effort in the future, plus keeping the CSS/templates from getting "hairy").

If you still feel that this is the right way to go, well ... we'll basically have to agree to disagree! Still, I'd really wish you take a good look at the documentation, examples, and arguments by people more experienced than you and me with an open mind before deciding.

aristippe commented 8 years ago

I did take a longer look at Bootstrap after my last reply and am a bit less hesistant to try it out. I still would prefer to take it in per element and customize per element rather than importing Bootstrap as a whole. If in the end, the entire Bootstrap is taken in, at least the customization would have been done per element already.

There was one thing I wasn't sure about so I was waiting a bit until I had a chance to look at it more. The table in list_view is using DataTables which allow for some future ajax. I'm not sure yet if ajax is the way to go, for instance being able to multi-select books in list_view and batch apply changes such as author or publisher. Perhaps keeping DataTables works for that, or if it's not done through ajax but some checkbox and form, divs rather than a table, perhaps through Bootstrap would work.

As for django-bootstrap3 or pure-pagination, I still liked pure-pagination in terms of implementation over bootstrap3 or any other pagination apps I've seen so far. I could look further if you had reservations about bringing in another package, though if we did choose a particular pagination over bootstrap3, pure-pagination does seem so far like the most well maintained of them.

aristippe commented 8 years ago

Since now that we can link files, there's the possibility that book_file might not exist and that will throw an error. It would be nice to show size; would the best way be to add something to context like file_exists?

Hmmm, probably just enclosing within something similar to {% if file %} would do the trick, in order to try to avoid passing extra cruft in the context?

I think {% if file %} might only check if the field has a value, and not if the file exists? There is also an older wontfix request for this issue, and that seems to suggest there is no built-in way at the moment. Perhaps a template tag would be the cleanest way? if that seems best, I'll give it a try.