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
298 stars 444 forks source link

Add the ability to show features on the homepage for OJS #9262

Closed NateWr closed 11 months ago

NateWr commented 1 year ago

Describe the problem you would like to solve Many publishers would like to highlight items on their homepage. For example, they want to showcase prominent articles or news in a visual "slider" or "carousel".

Describe the solution you'd like Add support for creating "featured" or "highlighted" items in the editorial backend. Themes can then show these items however they want. The default theme could show them in a slider or carousel.

It should be possible to create featured items at the journal or site level.

Who is asking for this feature? This is part of sponsored development work for a PKP|PS client, UNIANDES, who will use this feature in their custom theme.

Additional Information This will be added in lib/pkp, but only enabled for OJS, because OMP already has "spotlights" which do the same thing.

Pull requests stable-3_4_0: https://github.com/pkp/ui-library/pull/285 https://github.com/pkp/pkp-lib/pull/9270 https://github.com/pkp/ojs/pull/4030 (no support for OMP or OPS in 3.4)

main: https://github.com/pkp/pkp-lib/pull/9321 https://github.com/pkp/ui-library/pull/288 https://github.com/pkp/omp/pull/1458 https://github.com/pkp/ojs/pull/4038 https://github.com/pkp/ops/pull/567

NateWr commented 1 year ago

The following PRs add Highlights to OJS and the default theme. @asmecher would you or someone have time to review this?

https://github.com/pkp/ui-library/pull/285 https://github.com/pkp/pkp-lib/pull/9270 https://github.com/pkp/ojs/pull/4030

Notes:

  1. I called it a "highlight" and not a "feature" because there is already a database table in OMP called features.
  2. I used the Bootstrap 4 Carousel. This is quite old now, but the other components in the default theme use Bootstrap 4, so it aligns well with the existing theme dependencies. To get this to work, I had to update the existing utils.js file of the theme to Bootstrap 4.6's copy.

Examples:

Example of site-level highlights in the default theme on 1200+ pixel screen widths [Screencast from 30-08-23 16:37:44.webm](https://github.com/pkp/pkp-lib/assets/2306629/69a3a19f-2dbe-46dc-a378-0ff273a9dd3f)
Example of journal-level highlights in the default theme on a small mobile device [Screencast from 30-08-23 16:40:16.webm](https://github.com/pkp/pkp-lib/assets/2306629/ff2f6aee-013d-4fe5-a484-b44b462ccc00)
Example of managing highlights in journal backend [Screencast from 30-08-23 16:43:57.webm](https://github.com/pkp/pkp-lib/assets/2306629/3c02b1c4-e97f-482e-a8af-13614c32c3fc)
asmecher commented 1 year ago

Thanks, @NateWr! @jardakotesovec, could you have a look at the ui-library part of this (PR linked above)?

asmecher commented 1 year ago

@NateWr, I've added some reviews:

Is the idea to port this to OMP and OPS as well once the completed PRs are ready?

jardakotesovec commented 1 year ago

@NateWr Thank you! Its first time for me to look into this functionality, so bare with me. Couple of questions.

It should be possible to create featured items at the journal or site level.

Currently it seems its possible only for journal level or did I miss it?

Would not this be opportunity to extend the spotlights from OMP to be bit more general, so apart from actual products (book/article) they could also reference url, so we don't have two similar concepts to maintain?

That carousel looks quite good.. having bit of the retro vibes :-). To use some reference on carousel UX. Point from that article that I can relate is that featuring just one item does not motivate much to explore more. While displaying multiple items, with option to see another batch of items is more encouraging to explore the content as its more efficient. So kind of carousel with multiple items maybe would be better fit? Or quite common approach is just show couple of items with link to display all on dedicated page.

Maybe I am just missing some context about the use case - is this actually intended to feature articles? Or what would be usual thing to highlight?

Would be curious about @Devika008 thoughts.

Devika008 commented 1 year ago

Carousels may placate corporate infighting, but on large or small viewports, people often scroll past carousels.

Alternative to carousels are websites using a single hero image to indicate any highlight. @jardakotesovec having different sections scroll on a carousel is a good idea however I'm certain most users will scroll past. The idea that we have incorporated those bits in the carousel would discourage us from putting it in other sections around the website.

However I do understand the use case for having the carousels and some of the benefits it brings. I strongly suggest that if we are incorporating there are certain small UI components we need to incorporate so that nothing is missed and the benefit of the carousel is actually received.

  1. Include 5 or fewer frames within the carousel, as it’s unlikely users will engage with more than that. Limiting the number also helps with discovering the content, and finding the content in the carousel again later.

  2. It’s difficult to read small text and decipher small images, especially on mobile devices. And it’s never any good to cram a large, high-density image into a small region. The clearer the text and images, the more likely users are to engage and understand the intended points. I like how the Corning Museum of Glass has incorporated carousels pertaining to this point. A clear image with small but clear amount of text image

  3. Indicate how many frames are present and where the user is within the progression, to help people feel in control. Coldwell Banker was one of 10 winners of our Intranet Design Annual contest. Their intranet’s carousel design clearly showed important information: there are 5 frames, the content of each frame is basic, and it is easy to identify which frame is selected. image This is something I feel we can easily incorporate because dots with numbers at the bottom is not good wrt accessibility

  4. If offering a navigation button for each frame (rather than arrows to scroll through), ensure that each button looks different, and represents the frame. Make links and buttons large enough to decipher and click. image image if you see here the CTA button is different than the arrows used to navigate left or right. Also we should not be including an autoscroll. We wont ever be able to gauge the optimum time as people read and grasp content very differently.

I think if we are in our way able to incorporate these small things then the carousal will actually be beneficial to the user no matter the kind of content we are using it (be it sections or aticles)

ronste commented 1 year ago

Hi all,

I just came accross this issue when working on the update of our sliderHome plugin for OJS 3.4.

The sliderHome plugin is meant for images but also offers the option of having an HTML overlay above the image. It would be fairly easy to add an option for plain HTML slider content. This could natuarally also be taken from a template file.

We use the swiper bundle from http://swiperjs.com which offers lots of features. We only implemented the very basic ones though.

You can see the plugin in action at:

If you think this could be useful please drop me a line.

Best wishes, Ronald

NateWr commented 1 year ago

Thanks everyone. After reading your comments and discussing a little bit with Alec, my plan is to:

  1. Leave the stable-3_4_0 branch as-is, but remove the carousel. When people want to use highlights in 3.4, they'll need to use the feature flag and implement it in their own theme.
  2. When porting to the the main branch, make it available to OJS/OMP/OPS. Refactor OMP's spotlights to use these instead and remove spotlights for 3.5.
  3. For the default theme, use a different library for the carousel to incorporate better UX.

Regarding 3, @Devika008 I'm not sure how to integrate your suggestions. We don't have as much space in our default theme as there is in the examples that you provided. I'm also not sure how it will look on a mobile device. We probably can't enforce good art direction in how the carousel is used (for example, limiting how many slides they use, the length of text they write, or the position of the focus of images they choose). People will use this however they want.

That's why I kept the image and the text separate. I don't know how we can guess, for example, how tall the text will be. So it's very hard to know if we have space to show the next slides or whether anything other than the text will even be visible in a single screen. I'd love to see a mockup in mobile/tablet/desktop if you have an idea.

@ronste thanks for the recommendation of swiperjs. It looks well supported and has both vanilla js and vue options, so could be migrated if the team ever decides to bring Vue to the default theme.

asmecher commented 1 year ago

Thanks, @NateWr! I really appreciate the broad thinking here.

NateWr commented 12 months ago

Ok, all of the tests have passed now. The main PRs went stale, so I have rebased and pushed, but expecting the tests to pass. To recap, these changes:

@asmecher there are some outstanding PR comments, particularly around how the swiper.js dependencies are handled. So please take another pass through the PRs:

stable-3_4_0 https://github.com/pkp/ui-library/pull/285 https://github.com/pkp/pkp-lib/pull/9270 https://github.com/pkp/ojs/pull/4030 (no support for OMP or OPS in 3.4)

main https://github.com/pkp/pkp-lib/pull/9321 https://github.com/pkp/ui-library/pull/288 https://github.com/pkp/omp/pull/1458 https://github.com/pkp/ojs/pull/4038 https://github.com/pkp/ops/pull/567

asmecher commented 11 months ago

@NateWr, with apologies for the delay, I've re-reviewed the stable-3_4_0 PRs and they look good except for a few typos. I've also commented on the couple of outstanding discussions -- at this point I'm ready for you to push ahead with whatever seems best.

The main PRs will need some adaptation -- @jardakotesovec has moved us to Vue3, and @touhidurabir is working on rewriting our Slim routes to use Laravel instead, which hopefully will be merged next week. Jarda and Touhidur are probably best suited to adapting these PRs forward for those changes, so if you're working on a main branch that's taken from before the Vue3 upgrade was merged, please dust your PRs off to your satisfaction and we'll take it from there.

NateWr commented 11 months ago

@asmecher the tests are passing for stable-3_4_0 and if you're ok with the approach described in this comment, then it is ready for merge. The main tests have partially passed and I am optimistic the rest will pass. If so, I'd really appreciate a merge on this so that I can get on with other tasks. I appreciate you've got other things to worry about, though.

All of the PRs are listed in the original comment in the issue.

NateWr commented 11 months ago

The tests seem to have passed. The exception is the OPS main PR, but all tests passed except one, which appears to be related to the Vue.js PR, which was mentioned and fixed in Mattermost: https://mattermost.publicknowledgeproject.org/pkp/pl/3fe85oy5qfrwp8mu341fau4dqr

At this point, I think rebasing and running the tests again is just a waste of time. Unless there are any objections, can someone please merge these PRs?

asmecher commented 11 months ago

All merged :fireworks: -- @NateWr, is this ready to close?

NateWr commented 11 months ago

Woohoo! Thanks

asmecher commented 11 months ago

Whoops, it looks like not all aspects of this were hidden behind the Config::getVar('features', 'highlights') check. When I go to the Admin interface with a 3_4_0-3-level database (no highlights table), I get:

Next Illuminate\Database\QueryException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'ojs-stable-3_4_0.highlights' doesn't exist (SQL: select `h`.* from `highlights` as `h` where `h`.`context_id` is null order by `h`.`sequence` asc) in .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Connection.php:760
Stack trace:
#0 .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Connection.php(720): Illuminate\Database\Connection->runQueryCallback()
#1 .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Connection.php(422): Illuminate\Database\Connection->run()
#2 .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php(2706): Illuminate\Database\Connection->select()
#3 .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php(2694): Illuminate\Database\Query\Builder->runSelect()
#4 .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php(3230): Illuminate\Database\Query\Builder->Illuminate\Database\Query\{closure}()
#5 .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php(2695): Illuminate\Database\Query\Builder->onceWithColumns()
#6 .../ojs-stable-3_4_0/lib/pkp/classes/highlight/DAO.php(98): Illuminate\Database\Query\Builder->get()
#7 .../ojs-stable-3_4_0/lib/pkp/classes/highlight/Collector.php(64): PKP\highlight\DAO->getMany()
#8 .../ojs-stable-3_4_0/lib/pkp/pages/admin/AdminHandler.php(732): PKP\highlight\Collector->getMany()
#9 .../ojs-stable-3_4_0/lib/pkp/pages/admin/AdminHandler.php(202): PKP\pages\admin\AdminHandler->getHighlightsListPanel()
#10 [internal function]: PKP\pages\admin\AdminHandler->settings()
#11 .../ojs-stable-3_4_0/lib/pkp/classes/core/PKPRouter.php(334): call_user_func()
#12 .../ojs-stable-3_4_0/lib/pkp/classes/core/PKPPageRouter.php(277): PKP\core\PKPRouter->_authorizeInitializeAndCallRequest()
#13 .../ojs-stable-3_4_0/lib/pkp/classes/core/Dispatcher.php(165): PKP\core\PKPPageRouter->route()
#14 .../ojs-stable-3_4_0/lib/pkp/classes/core/PKPApplication.php(387): PKP\core\Dispatcher->dispatch()
#15 .../ojs-stable-3_4_0/index.php(21): PKP\core\PKPApplication->execute()
#16 {main}
  thrown in .../ojs-stable-3_4_0/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Database/Connection.php on line 760

@NateWr, could you have a look?

NateWr commented 11 months ago

@asmecher sorry about that! Should be fixed in the PRs below:

PR: https://github.com/pkp/pkp-lib/pull/9426

Tests: https://github.com/pkp/ojs/pull/4070

asmecher commented 11 months ago

Merged -- thanks, @NateWr!