Skip to content

Proposal: Deprecate Fotorama #33

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

Closed
wants to merge 3 commits into from
Closed

Proposal: Deprecate Fotorama #33

wants to merge 3 commits into from

Conversation

DrewML
Copy link
Contributor

@DrewML DrewML commented Oct 15, 2018

Status (1/4/19): This proposal is still in an active state, but is currently held up on some other priorities. Before this can be closed out and worked, a new library with a comparable feature set needs to be decided on.

Rendered Proposal

I'd like to hear from Partners, SIs, and anyone else working on front-end for Magento stores on a day-to-day basis. I'd imagine there are some common slider/gallery libs in use - would love to hear suggestions.

@DrewML DrewML added the open discussion Proposal or design document is open for public discussion label Oct 15, 2018
@brendanfalkowski
Copy link

@DrewML — I never liked the Fotorama choice. Always feels clunky and a little (too soon?) https://sears.com.

Been using http://photoswipe.com/ for a while. Only pinch-zoom I've seen that feels good on touch devices, and I like how its pieces are modular.

@johnhughes1984
Copy link

johnhughes1984 commented Oct 15, 2018

@DrewML There is a proof of concept (POC) gallery replacement using flickity created by one of our devs at Fisheye: @robaimes - https://github.com/robaimes/module-product-gallery

Video demo: https://gfycat.com/WelltodoAppropriateCony

It's been on our list to create a full implementation from Rob's POC for some time. I would also note the original focus of this extension was purely to replace fotorama due to it's inflexibility, extensibility and lack of responsive friendly nature as well as poor UX. However the POC definitely has major rendering speed improvements over fotorama in addition to the above.

Flickity does however not meet your 5KB gzipped / compressed limit so appreciate may not be viable (comes in at ~13KB - https://unpkg.com/flickity@2.1.2/dist/flickity.pkgd.min.js)

@dankoz51
Copy link

dankoz51 commented Oct 16, 2018

but for some zoom and tablet type of features, I think I would be more in favor of just using as much css and html as possible.

Things like photorama (have not looked at flicket yet) are created to make the html/css markup easier on an html coder, that way the create simple symantec html, and have it transformed for them, we dont need that we need it to perform, who cares how hard the css and html are.

@jackmcpickle
Copy link

Agreed with @brendanfalkowski on photoswipe. But for something more simple to cover swiping and sliding I use http://owlcarousel2.github.io/OwlCarousel2/

@elioermini
Copy link
Member

Agree we use swiper http://idangero.us/swiper/ and works well either desktop and touch, in addition we first render the static base image then once swiper loads will load the gallery, in this way the the first image loads same time the rest of the product content.

@DrewML
Copy link
Contributor Author

DrewML commented Oct 16, 2018

I expect this thread will get noisy, so I'm pulling out each of your lib suggestions to this document.

Keep them coming, and I'll start looking through them soon to see which lib(s) fit the bill

@AngelBS
Copy link

AngelBS commented Oct 16, 2018

I agree on this proposal, and i'll like to suggest Slick.js, i've been using it for a while and it's easy to use and has a lot of options

http://kenwheeler.github.io/slick/

@DrewML
Copy link
Contributor Author

DrewML commented Oct 16, 2018

@AngelBS I've seen Slick used in the past, but it's massive and won't fit the size requirements in this proposal. The minified version is actually 2kb larger than the version of Fotorama we're using

@dankoz51
Copy link

I expect this thread will get noisy, so I'm pulling out each of your lib suggestions to this document.

Keep them coming, and I'll start looking through them soon to see which lib(s) fit the bill

I think we should add "None" as an option. I dont see why we need a lib to bloat the load and have support for lots of "features" that are never used.

At the very least my requirement would be that it loads with no DOM modification at all, only register events for zoom, scroll, pinch etc.

@Igloczek
Copy link

Igloczek commented Oct 17, 2018

@jackmcpickle Owl is not actively maintained (it's not a first time, AFAIK they even changed maintainer(s) like 2 years ago), weight too much, plus require jQuery (it's not in the requirements, but to make it really fast, will be nice to use something that can handle own shit, without asking colleagues for help :) )

@vasilii-b
Copy link

vasilii-b commented Oct 17, 2018

@Igloczek,

@jackmcpickle Owl is not actively maintained (it's not a first time, AFAIK they even changed maintainer(s) like 2 years ago), weight too much, plus require jQuery (it's not in the requirements, but to make it really fast, will be nice to use something that can handle own shit, without asking colleagues for help :) )

I'm with 2 hands up for not using Owl. Have some unpleasant memories with it. It's better to use Slick in this case.

Also, I'm with @dankoz51 on the adea to add the "None" option. Will be really nice to have an option in the system configuration to turn off the product gallery stuff.

@jackmcpickle
Copy link

@Igloczek and @vasilii-b agreed. I throw my vote in with Slick which I have also used.

@brendanfalkowski
Copy link

brendanfalkowski commented Oct 17, 2018

Owl is not actively maintained (it's not a first time, AFAIK they even changed maintainer(s) like 2 years ago), weight too much, plus require jQuery (it's not in the requirements, but to make it really fast, will be nice to use something that can handle own shit, without asking colleagues for help :) )

I agree with this. The new maintainer (or old one IDK) nuked the Owl v1 site so the docs are lost.

Possibly not the right thread for this, but IMO selecting an image presentation library rarely depends on performance in practice. It matters obviously, but nearly every library is 10+ KB of JS. If you lazy-load / lazy-init the advanced functionality there's not a huge difference between 10 and 30 KB. [...I know there is but it doesn't change the decision].

The decision usually rests on behavior supported by the library. We all have favorites that do certain behaviors well. But there's usually only 1-2 that do "behavior X" well and are reasonably customizable.

  • TouchCarousel nailed swiping to advance (for its time)
  • PhotoSwipe nailed pinch/zoom in modal mode
  • Owl v1 nailed scaling item count to "fit" the row by breakpoint first
  • Slick stepped over Owl v1 before work resumed on Owl v2

I think we should be voting for the intended behavior of the Fotorama replacement. I haven't found an all in one library that works, so usually my projects end up with three libraries dedicated to:

  • a traditional content-width carousel for 5-25 items (ex: upsell)
  • a PDP-focused media zooming/panning modal
  • a PDP-focused primary + secondary images swapper (not really a library, 3 lines of JS)

Doing this makes it easier to focus on the behavior desired and keeps file sizes down because one library isn't being asked to move the planets and do each of those things well.

— — — — — — — — — —

Aside: David DeSandro who makes https://flickity.metafizzy.co/ is a nice guy and has built a business supporting JS libraries with fair licensing for years. All his stuff is worth a look.

@robaimes
Copy link

As a note, I wouldn't necessarily recommend Flickity for this, as it's not particularly well adapted to being used as a gallery. For example, it doesn't have a vertical slider option. The main advantage of why I chose to use Flickity was because it is highly customisable via CSS instead of JS. The POC posted here is just a prototype. I did make a build using slick, and it works just as well.

Not sure the 5KB requirement is entirely achievable but would it not make sense to use Slick, assuming it's still used in the new PagerBuilder module?

@DrewML
Copy link
Contributor Author

DrewML commented Oct 17, 2018

It seems like we're all in agreement that Fotorama needs to go, which is good! Pretty rare that everyone in a Github thread reaches a consensus 😂

The open question is obviously what a replacement is going to be. It's clear that what I'm missing right now is a minimum viable features list.

Because I don't work on client sites, I'm far from the best person to draft up a list of what use-cases we need to cover. Would someone here be willing to draft up a list to be included in this proposal? Maybe @brendanfalkowski or @robaimes?

@DrewML
Copy link
Contributor Author

DrewML commented Oct 17, 2018

Not sure the 5KB requirement is entirely achievable

@robaimes That's very possible, and I'm open to changing it. But, I'm starting aggressively small, and we'll adjust if it's not possible. Tight constraints are important here to ensure we really think through this decision.

I believe nailing down a list of the minimum viable feature set will help us determine how feasible that 5KB target is.

@DrewML
Copy link
Contributor Author

DrewML commented Oct 23, 2018

I pushed a change that added a list of minimal viable features for a replacement lib.

Kindly requesting review/suggestions from everyone that frequently deals in this area day to day 🙂

@brendanfalkowski
Copy link

@DrewML — In my todo tabs, will get some ideas into that doc.

@brendanfalkowski
Copy link

brendanfalkowski commented Oct 24, 2018

Ok, here's my take on "a client would actually approve this UX-wise" more than a minimal simulacra of most eComm sites. I couldn't get away with skipping the finer points in a real project and it doesn't help developers to have an MVP with rudimentary functionality they can't extend to great UX. If that's an aim, then I'd ship the easiest thing to remove because it'll be replaced regardless.

These are the questions and implementation points that would be addressed in a site's design plan. If the MVP skips these for the sake of minimalism, the behavior would feel incomplete relative to other sites.

I wouldn't aim for a single library that solves "product carousel + media swapper + media zoomer" in one. They're each simpler + smaller to spec individually (needing no library in some cases).

Some of these lists include "this or that" features where both approaches likely won't be implemented for the same client (ex: snapping or frameless carousel). But that decision satisfies a design constaint that differs across clients, and there isn't a "right" approach.

Product List Carousel — Ex: recently viewed, customers also bought, etc

  • Keyboard accessible
  • Items per frame "snapping" to container width per breakpoints in instance's config
  • Frameless x-scrolling, so an item edge falls across the container hinting there's more
  • Pagination as indicator of volume and nav element (dots or numbers)
  • Nav left + right per frame by clicking button
  • Nav left + right per frame by swiping across items
  • Infinite carousel mode (or not) by wrapping when reaching the end
  • Lazy-loading images as frame enters container (full HTML in DOM)
  • Lazy-loading additional frames via AJAX for performance (first-frame only in DOM)

Product Page — Primary + Secondary image switcher

  • Keyboard accessible
  • Render primary image from HTML
  • Option to lazy-load secondary images
  • Render all secondary images as multi-line list of thumbnails
  • Render all secondary images as single-line carousel of thumbnails
    • Swipe to move along line
    • Scroll-x to move along line
    • Nav left + right button to move along line
  • Swap secondary image to primary slot
    • By clicking secondary image
    • By swiping across primary image (1 item carousel)
    • By clicking buttons for nav left + right (1 item carousel)
  • Option to require/disallow manipulating product options to swap/load alternative swatches. Depending on volume of swatches + media this either hurts or helps UX.
  • Source for full-size primary + secondary images is processable by 3rd-party (ex: pin to Pinterest)

Product Page — Zoom Viewer

  • Keyboard accessible
  • Click to expand (rarely: hover to expand, always feels clunky on touch devices)
  • When expanded, render highest resolution available
  • When expanded, supports pinch to zoom
  • When expanded, supports (+) and (-) buttons to zoom
  • When expanded, supports mouse wheel to zoom
  • When expanded, supports double-tap to zoom
  • When expanded, supports nav left + right buttons to cycle secondary images
  • When expanded, supports swiping to cycle secondary images
  • When expanded, supports infinite carousel to wrap last ➔ first image
  • On load, preload the primary image at full resolution to improve perceived performance
  • When expanded, preload N+(1-3) full-size assets to improve perceived performance

@DrewML
Copy link
Contributor Author

DrewML commented Nov 1, 2018

@brendanfalkowski Thanks for doing that!

I'm a little bit confused about the section for Product List Carousel. I did not think that features like "Recently Viewed" and "Customers also Bought" used the Fotorama carousel - am I incorrect?

@brendanfalkowski
Copy link

@DrewML — I don't think it is either.

In most real builds we have a generic carousel library, and would make the UI for "recently viewed carousel" and "PDP primary image carousel" using the same carousel library if possible.

I added those ideal specs for the "product list carousel" concept just because it might help to research that a carousel library selected for swiping across PDP images also be usable for another UI piece. Just easier for devs to lookup one library config.

@magento-engcom-team magento-engcom-team deleted the replace-fotorama branch November 9, 2018 11:28
@sidolov sidolov restored the replace-fotorama branch November 9, 2018 13:41
@sidolov sidolov reopened this Nov 9, 2018
@guz-anton
Copy link

Hi @DrewML,

Requirements for a replacement

  • ...
  • Should not be initialized using the data-mage-init or x-magento-init functionality in core

What are the concerns about initializing mechanism?
What will be the suggestion to do it instead of data-mage-init?

@DrewML
Copy link
Contributor Author

DrewML commented Dec 11, 2018

@guz-anton

What are the concerns about initializing mechanism?
What will be the suggestion to do it instead of data-mage-init?

For the gallery, I want to avoid using mage-init to initialize because it is one of the most critical pieces of above the fold content on a product page, and the order of mage-init widget execution is not deterministic (when not bundled).

To help illustrate, the screenshot below shows 2 different tabs from the same store, and the order that widgets were initialized. Note that the order is not the same between different page loads:

image

Initializing imperatively gives us more precise control over when execution of the widget begins.

@kandy
Copy link
Contributor

kandy commented Jan 4, 2019

@DrewML, can I ask you to describe the real problem? It is normal for asynchronous code to load in a different sequence. It is only important that the code be executed only when all dependencies are available, and the order of loading itself is not important.

@DrewML
Copy link
Contributor Author

DrewML commented Jan 4, 2019

@kandy Described in this performance write-up referenced in the proposal doc

@kandy
Copy link
Contributor

kandy commented Jan 5, 2019

@DrewML, all that you say in the bug definitely make sense. But I personally think that it is will be the match valuable to improve the declarative mechanism of initialization instead of adding inline javascript

@DrewML
Copy link
Contributor Author

DrewML commented Aug 29, 2019

To those still watching: for now, I am going to withdrawal this proposal.

There were two primary motivators for me when I wrote the proposal:

  1. Speeding up product page image performance
  2. Improving DX

For 1, magento/magento2@56e56ce greatly improved the performance of initial product page rendering, allowing the browser to begin fetching the product photo immediately. The work we're doing on bundling with baler will continue to improve these numbers

With regards to DX, I've had a slight change of opinion. With pwa-studio being worked on as the future of Magento front-ends, I am more hesitant to make breaking changes to current themes unless the benefits are too hard to ignore. In this case, the benefits are more debatable, since those that don't want to use Fotorama have many free (and paid) alternatives for m2.

Thanks to everyone that participated and provided input on this!

@DrewML DrewML closed this Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open discussion Proposal or design document is open for public discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.