search

pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
307 stars 447 forks source link

Handle frontend display of chapter files better #1428

Closed NateWr closed 7 years ago

NateWr commented 8 years ago

Monographs with chapter file associations should display those files alongside the chapter. Also, long author lists should be displayed better.

NateWr commented 8 years ago

I've made a number of changes that should improve things. However, I'm waiting to discuss a few things with Alec before issuing a PR.

Before:

omp-chapter-files-old

After:

omp-chapter-files-new

Changed:

beghelli commented 8 years ago

I like it. What do you think about still using the publication format name for all files? See chapter 2: I think it's confusing to not know the publication format of the two first files. Maybe somethin like PDF - filename would solve that confusion.

2016-05-05 12:04 GMT-03:00 Nate Wright notifications@github.com:

I've made a number of changes that should improve things. However, I'm waiting to discuss a few things with Alec before issuing a PR.

Before:

[image: omp-chapter-files-old] https://cloud.githubusercontent.com/assets/2306629/15046985/8c81980c-12da-11e6-9ecd-08b3e2fc757f.png

After:

[image: omp-chapter-files-new] https://cloud.githubusercontent.com/assets/2306629/15046988/93771966-12da-11e6-91bf-6f7463030f7d.png

Changed:

  • Moved the author biographies down below
  • Used author affiliation (instead of role) at the top
  • When more than 4 authors exist, it displays them in one line instead of each on their own line
  • Files associated with chapters display below their chapter. If only one chapter file exists for a publication format, it uses the pub format name. Otherwise it uses the file name.
  • Files not associated with chapters display on the right as before.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/pkp/pkp-lib/issues/1428#issuecomment-217178474

NateWr commented 8 years ago

I agree it's not optimal to lose the publication format. But I don't like jamming it into a single string -- I don't think it will look very professional.

I consider it a bit of an edge case where we're not going to get an optimal display anyway. One idea would be to design a special segmented button, so that the publication format is displayed within the button (just not concatenated onto the string). But I think we run into a lot of potential problems with that: file names and publication formats can be very long. If it's just PDF it might not be problematic. But if someone puts "Downloadable HTML" or "Online Reading Platform" in it starts to get pretty fuzzy.

Another approach is to display such files (where a pub format has more than 1 file) like the "Research Transcripts" file on the right in the second screenshot. But I worry that we're really undermining the visual coherence of the chapter list that way.

I'm just really hesitant to jam more logic into the template for edge cases. And since editors can adjust the file name they have an "out" for exceptional circumstances. But if it's reasonably common and causes problems for a lot of presses I'm happy to take another stab at it.

NateWr commented 8 years ago

pkp-lib https://github.com/pkp/pkp-lib/pull/1429 omp https://github.com/pkp/omp/pull/283

@alec I issued PRs to master. Should I issue them to any other branch?

I decided to deliver a flat array of files and then write a smarty helper function that would pluck files from the list by various parameters. Although it ends up in some repetitive looping, I think it makes the template logic much more readable and provides some flexibility for people who want to quickly get an array of specific kinds of files.

I think we'll still find not ideal displays for some edge cases, particularly because we don't really know much about the left-over files. But the {pluck_files} function should make it easier to account for this in individual setups.

I also went ahead and stripped out any publication formats that aren't available. The existing code limited them by approval setting, but not availability.

I'm also curious about why the $availableFiles smarty variable is only being assigned from within a conditional which seems related to eCommerce:

https://github.com/pkp/omp/blob/master/pages/catalog/CatalogBookHandler.inc.php#L100-L122

Just thought I'd check to see if that made sense to you?

asmecher commented 8 years ago

@NateWr, we require a payment plugin to be configured, even if that's only the manual payment plugin, which gets a default configuration. So that conditional should always evaluate true unless something has been actively misconfigured.

This is a lot cleaner code-wise than what we used to have.

Marching orders!

jmacgreg commented 8 years ago

Done the cherry-pick!

stranack commented 8 years ago

The looks much better.

We should also think about a better way to display the author bios, but I don't think that is a priority for 3.0.

asmecher commented 8 years ago

Great, I think we have this covered for the moment!

NateWr commented 8 years ago

@jmacgreg The CSS cache needs to be deleted so it's recompiled on the SFU installation. And it looks like the JS is still not getting concatenated -- 20+ second load times on the site over here.

NateWr commented 8 years ago

Also, that empty space below the cover image shouldn't be there if there are no non-chapter files. @stranack can you take a screenshot of the Publication Formats grid for that monograph? That might give me a hint as to what's going on.

jmacgreg commented 8 years ago

@NateWr done and done - all good now? (Note to self to add the minified thing to our install scripts)

NateWr commented 8 years ago

@jmacgreg 2.5s now and CSS looks good. Thanks!

stranack commented 8 years ago

@NateWr I deleted some non-chapter files (and filed an issue that we should be able to edit file types), so this may be some leftovers from that. I've signed you up for the installation so you can take a look around.

NateWr commented 8 years ago

Thanks, that clarified things quickly. I've issued a PR that should address that:

https://github.com/pkp/omp/pull/284

@stranack I noticed that you created a "Chapter" for the whole book. Is there a reason that you didn't want the whole-book files to appear on the right? They'll get a larger button presentation that's more prominent on the top right, and it will keep the data clean (rather than adding fake chapters to try to effect the presentation).

selection_039

stranack commented 8 years ago

Nope, no reason. I agree that this would be a better way to handle it. I'm assuming I'll be able to label the button something like "Complete Book PDF".

NateWr commented 8 years ago

You won't actually be able to name the button.

This was a difficult choice and I'm open to alternatives. When there's just a single file in a pub format, it uses the pub format name (eg - the chapters say PDF instead of the full file name). It falls back to the file name if there are multiple files per pub format.

So in your case, since there'd only be one PDF for the whole book, it would display the pub format. This may not be ideal for you, but it makes the most sense for someone like Smithsonian, who only ever distributes a single file.

This might be a case where we need to add an option in the file settings, since I don't see a good way to resolve this programmatically:

Alternatively, we could add a text field to the file settings which was a download button label.

If you think this needs doing, re-open this issue so we don't lose track of it.

asmecher commented 8 years ago

I'm hesitant to add more names or options to this. I suspect the amount of pain in working with it as is will be equal or less than that for adding new controls etc.

IMO identifying the file type (e.g. PDF) is better done graphically (e.g. by using PDF iconography) separately from the label text.

NateWr commented 8 years ago

The problem with identifying the file type graphically is that most file types don't have a graphic that can be used with clarity. PDF is a rare instance of a recognizable icon. But what about HTML, ePub, the eLens Reader?

asmecher commented 8 years ago

I thought that sort of thing was part of plenty of standard icon sets, but now I'm definitely jumping up and down on your toes, Nate -- I'll back whatever you recommend here.

asmecher commented 8 years ago

@NateWr, I've merged https://github.com/pkp/omp/pull/284, thanks -- but leaving this open as I don't think it's ready to be closed.

NateWr commented 7 years ago

It's not clear to me what would be next steps for this issue. If there are additional improvements we want to make at this stage, let's put them in a new issue.

cc @stranack, did you have any specific requests for how to change the file display?