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

150 list and grid feedback #202

Merged
merged 49 commits into from Jan 17, 2018

Conversation

jonas-atmire
Copy link
Contributor

This PR connects to #150
It uses the ViewMode to switch between displaying "list" and "grid"
(Currently, only the pre-existing viewmode switcher on the search page is used)

Additionally, this also moves some packages to the "shared" module instead of directly under the app directory

@ghost ghost assigned jonas-atmire Nov 22, 2017
@ghost ghost added the needs review label Nov 22, 2017
@tdonohue
Copy link
Member

@jonas-atmire : This is currently showing build errors in Travis: https://travis-ci.org/DSpace/dspace-angular/jobs/309526382#L4099

@artlowel
Copy link
Member

artlowel commented Dec 1, 2017

@tdonohue looking at the travis build output, those errors don't seem to be caused by this PR, they're in files that aren't affected by this PR.

I think that #207 and #208 made the travis rules stricter and more likely to fail, and this PR is the first one to use them so it fails on errors that were already there.

In any case, those errors prevent the actual tests on this PR from even running, so they should still be fixed before the PR can be reviewed.

@jonas-atmire
Copy link
Contributor Author

The initial failure of the build(/test) was my doing.
During the merging my IDE seems to have made an incorrect path for an import
9b81210#diff-1397f93dd840039a0989edfdbf4bfe4eL15

This broke the spec-bundle.js imports.

The tests are now running, but some additional non-pr related tests still show some errors (that do not break the actual test run).
I'll fix these as well

@tdonohue
Copy link
Member

I finally got around to testing this today. Overall, it works well (thanks @jonas-atmire!). However, I did notice two issues in my tests:

  1. The grid view seems to loose pagination tools (at least on my screen). Pagination only exists in the list view, and when I switch to grid view, I just see Now showing items at the top (with no numbers) and the pagination tools at the bottom are gone.
  2. After switching to the grid view, if I go back to the list view, the pagination tools now oddly paginate after 12 items (when it usually defaults to 10). So, I now see Now showing items 1 - 12 of 20 at the top of the list view.

Beyond those two minor issues, this seems to work well.

@artlowel
Copy link
Member

  1. That was part of the spec. 12 divides by more numbers than 10, so the chances of having a full bottom row, and therefor a page that looks balanced, are greater. I told jonas to switch to the nearest multiple of 12 from the original rpp.

@tdonohue
Copy link
Member

@artlowel : It's quite off putting to see that change occur automatically when switching back and forth, but I understand better now. I just wonder if we should auto-switch back to 10 when you move back to "list view" (since obviously it's auto-switching to 12 when you move to grid view).

@artlowel
Copy link
Member

@tdonohue yes, it should definitely auto-switch back. If it doesn't that's a bug

@jonas-atmire
Copy link
Contributor Author

Currently, the pagination on the grid view has been fixed.
Additionally to this, the results per page is now updated according to the viewmode being used.

Another thing that has to be fixed is the following.
When changing from list to grid view, the results per page options in the sidebar still needs to update

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Retested this today, and it works great now. Both issues I previously found have been resolved.

@artlowel artlowel merged commit 1a1cc3a into DSpace:master Jan 17, 2018
@ghost ghost removed the needs review label Jan 17, 2018
@tdonohue tdonohue added this to the 7.0preview milestone Jan 26, 2021
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

3 participants