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

Add support for grid view #986

Closed
wants to merge 4 commits into from

Conversation

gpapin
Copy link

@gpapin gpapin commented Jul 29, 2019

Not too sure if I should base this on develop or master so feel free to edit.
This is pretty much done but still need to address 2 issues

  • Fix the alphabetical alphabetical sorting asc and desc in the cover view
  • Figure out what B,A <-> A B does and fix it in the cover view
  • Fix tooltip alignment on large displays

Feedback and pointers welcome.

@OzzieIsaacs
Copy link
Collaborator

The B,A <-> A B shouldn't be visible. It's made for the authors view (displaying surname, first-name or first name surname)

@gpapin
Copy link
Author

gpapin commented Jul 29, 2019

Ah ok that makes sense! Since the grid view doesn't support author at the moment I'll remove it entirely.

@gpapin gpapin mentioned this pull request Jul 29, 2019
@gpapin
Copy link
Author

gpapin commented Jul 29, 2019

I think I found a solution, using the the isotop lib that is included should solve all my problems.

@gpapin
Copy link
Author

gpapin commented Jul 29, 2019

Ok the default theme works but in Caliblur the sorting is still broken...

@gpapin
Copy link
Author

gpapin commented Jul 29, 2019

Ok so the 2 remaining issues are linked

  1. On the default theme: I use the .book class on each items filtering through isotope works fine but the tooltips are a bit off when the display is too wide, presumably because isotope is making each book element a bit wider to fit the screen (instead of using a flex-display for example).

  2. On Caliblur theme: the tooltip seems to work because the theme doesn't use a fixed number of items per rows so the book don't expand and the tooltip is where it should be. However there is some other css rules that mess with isotope that breaks the filtering... if I change my book css class to item for example the filtering start working again! But of course that breaks the tooltips as we see the same issue that we get from the default theme...

@OzzieIsaacs
Copy link
Collaborator

What about the crazy idea of having the badge in the upper left corner:
grafik

@OzzieIsaacs
Copy link
Collaborator

The sorting problem arises from these lines in caliblur.css.min:

.book {
    width: 225px;
    max-width: 225px;
    //position: relative !important;
    //left: auto !important;
    //top: auto !important;
    //-webkit-transform: none !important;
    //    -ms-transform: none !important;
    //        transform: none !important;
    min-width: 225px;
    display: block;
}

If you comment out the lines like in the code snippet the searching works

@gpapin
Copy link
Author

gpapin commented Aug 4, 2019

Thanks for looking in to this. I'll try out your fix and see if that affects anything else. I would be curious to know why those rules were there in the first place, but it this fist it I'll probably override it just in the grid view so it won't affect any other places this is used.

Regarding the left alignment, like you said this is crazy XD.
Seriously though, I think it's against common practices, in the western world (were we read from left to right, top bottom), any badges you'll see over the internet are on the right hand side of what it represent. Changing that might be confusing.
On the other hand, this would be a very small change and maybe one that could be implemented in a custom theme ? I've seen that there are already some discutions about theme and this isn't easy but what I've seen in some other app is rather than changing the whole theme drastically like Caliblur does, some app let you add custom css that runs after everything else. This is where I would put those changes.

@OzzieIsaacs
Copy link
Collaborator

OzzieIsaacs commented Aug 5, 2019

The thing I realized about the css, it's shortening the titles of the book, without the css the book titles can be very long and go into the next book.

@OzzieIsaacs
Copy link
Collaborator

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 19
           

Complexity increasing per file
==============================
- cps/ub.py  1
- cps/static/js/filter_grid.js  2
         

See the complete overview on Codacy


var $list = $('#list').isotope({
itemSelector: '.book',
layoutMode: 'fitRows',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


$("#asc").click(function() {
$list.isotope({
sortBy: 'name',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


var $list = $('#list').isotope({
itemSelector: '.book',
layoutMode: 'fitRows',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

var $list = $('#list').isotope({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itemSelector: '.book',
layoutMode: 'fitRows',
getSortData: {
title: '.title',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data = {};
data[target] = view;
console.debug("Updating view data: ", data);
$.post( "/ajax/view", data).done(function( result ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codacy Issue found: 'data' is not defined.

layoutMode: 'fitRows',
getSortData: {
title: '.title',
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -239,4 +239,17 @@ $(function() {
$(".discover .row").isotope("layout");
});

$(".update-view").click(function(e) {
var target = $(this).data('target');
var view = $(this).data('view');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/

var $list = $('#list').isotope({
itemSelector: '.book',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -239,4 +239,17 @@ $(function() {
$(".discover .row").isotope("layout");
});

$(".update-view").click(function(e) {
var target = $(this).data('target');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/

var $list = $('#list').isotope({
itemSelector: '.book',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var $list = $('#list').isotope({
itemSelector: '.book',
layoutMode: 'fitRows',
getSortData: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


e.preventDefault();
e.stopPropagation();
data = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codacy Issue found: 'data' is not defined.

}
});
// We need to trigger the resize event to have all the grid item to re-align.
window.dispatchEvent(new Event('resize'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data = {};
data[target] = view;
console.debug("Updating view data: ", data);
$.post( "/ajax/view", data).done(function( result ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.stopPropagation();
data = {};
data[target] = view;
console.debug("Updating view data: ", data);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.preventDefault();
e.stopPropagation();
data = {};
data[target] = view;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codacy Issue found: 'data' is not defined.

itemSelector: '.book',
layoutMode: 'fitRows',
getSortData: {
title: '.title',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.stopPropagation();
data = {};
data[target] = view;
console.debug("Updating view data: ", data);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codacy Issue found: 'data' is not defined.

@gpapin
Copy link
Author

gpapin commented Jan 19, 2020

I like that! I'm going to update at some point.
Is there a way to run it locally so I can test I haven't missed anything before pushing ?

@OzzieIsaacs
Copy link
Collaborator

Partly: Codacy is using different "linter" to do the checking.
For javascript it's ESLint and there is a config file in the root folder. For python code I don't have a config.
I'm sorry about all the emails, this was better in the past

@gpapin
Copy link
Author

gpapin commented Jan 19, 2020

Yeah don't worry gmail keeps it tidy in a thread anyway :)
I'm under the water at the moment but I haven't forgotten this feature by the way.

It would be nice to have a make test or make lint that would run the lint in a docker container. This is what I got for my other projects and it prevent me from having to go back and forth

pthiben pushed a commit to pthiben/calibre-web that referenced this pull request Apr 19, 2020
…ad of top right: each element's width in the list seems to be double the size of the image, so right alignment gets tricky. janeczku#986 (comment) suggests top-left alignment which does make a lot of sense. Using this for now. Maintainer might have a better idea to fix it
pthiben pushed a commit to pthiben/calibre-web that referenced this pull request Apr 19, 2020
…ad of top right: each element's width in the list seems to be double the size of the image, so right alignment gets tricky. janeczku#986 (comment) suggests top-left alignment which does make a lot of sense. Using this for now. Maintainer might have a better idea to fix it
@gpapin
Copy link
Author

gpapin commented May 26, 2020

I guess this can be close now that #1306 has been merged

@gpapin gpapin closed this May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants