Feature/migrate slider component to flickity library

[AdrianFiroiu]

This (currently) draft PR proposes the migration of the slider component and the associated stories to the Flickity library. For the reasons outlined below the changes are a WIP so I have made the minimum amount of changes necessary until this point.

I have opted for importing the following React Flickity component and utilizing it instead of the Glider wrapper. However I have run into a blocking issue where some options are not enabled / disabled properly within the storybook.

For example, the draggable option toggle does not seem to work within the component when toggled even though in the options prop it is set to true when toggled. A workaround for this particular case was using the reloadOnUpdate prop of the new component (despite it being a costly operation).

But, unfortunately, this solution does work for any other option toggle and I think we should discard it entirely. If we also reload the storybook page with the toggle arguments passed then the options work as expected.

Could you please also have a look at the changes in case I have missed anything regarding a solution? Thank you 🙂

[kuserich]

Thank you, @AdrianFiroiu . As far as I can tell, this behavior seems to be by design and I don't see any issues keeping reloadOnUpdate for the storybook component. I assume we will also require the property in the Edit component for the same reason. In the save function, I think we don't need it. CC @mahdiyazdani @gooklani

Currently it's not possible to pass this property down to Flickity from Slider. I would thus be in favour of extending the component accordingly and I would also be in favour of implementing more options from the library like freeScroll and wrapAround.

[mahdiyazdani]

Thank you, @AdrianFiroiu 👍 It appears that enabling reloadOnUpdate causes a noticeable flicker in the UI as by design with this property set, all DOM nodes associated with the slider are destroyed and recreated. I fear that this would cause a huge jump effect in reloading the "Slider" block heigh when later being used with rich or imagery content populated.

I propose leaving this property off by default and leaving the decision to enable it to the component's consumer where the slider's reloading could be justified.

[kuserich]

Thank you, @AdrianFiroiu 👍 It appears that enabling reloadOnUpdate causes a noticeable flicker in the UI as by design with this property set, all DOM nodes associated with the slider are destroyed and recreated. I fear that this would cause a huge jump effect in reloading the "Slider" block heigh when later being used with rich or imagery content populated.

I propose leaving this property off by default and leaving the decision to enable it to the component's consumer where the slider's reloading could be justified.

I'm afraid that we will need it in the Edit component. Otherwise the slider is not updated if the options are changed. At least, I can confirm that a single slider in the editor does not appear to exhibit any flickering on option updates on my local (although the slider is refreshed and thus returned to index 0).

[kuserich]

I'm afraid that the issue is even worse than originally anticipated. I've just been playing around with the current changes and have noticed that Flickity / Flickity React does not properly update options and does not rerender the slider upon option updates. More specifically, if an option like autoPlay is toggled, there is no change in the underlying Flickity slider (even if reloadOnUpdate is enabled) and thus no change in the rendered component. There also doesn't seem to be a built-in function that could be used easily (only a combination of flkty.destroy() with a new instantiation).

Another more elegant and simpler yet still a little hacky solution would be to recreate the entire component within React (for instance by changing key on every update). This also does not seem to exhibit the flickering behavior in the majority of tests on my local. CC @mahdiyazdani

[AdrianFiroiu]

After looking further into the issue it seems to be significantly more complicated than anticipated. The issues lie with both the react component and the Flickity library itself.

Starting with the react component - we can see withing Storybook that the Draggable option works once toggled but only because it is set properly within the componentDidUpdate via this block:

   this.flkty.options.draggable =
      draggable === undefined
         ? children
            ? children.length > 1
            : false
         : draggable;

Extending this to other options does fix them, such as:

this.flkty.options.autoPlay = autoPlay === undefined ? false : autoPlay;

But will not work for markup changes such as pageDots or prevNextButtons even with flkty.destroy(); and a new instantiation in my case.

For the Flickity library itself the problem is known and discussed in these 2 PRs, PR381 & PR488, where the general gist is that initializing the slider has no issues but changes to the cells or props cause React to lose the reference to the children and throw an error we encounter as well: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node. The solution proposes adding a new noDomMod option by which React can handle the .flickity-viewport and .flickity-slider nodes while allowing Flickity to handle everything else.

Implementing the changes proposed there did not initially fix the issue locally for me, although I will continue testing them. Could you please let me know your thoughts on the best way to move forward? Do you think it would make sense to start implementing our own react integration instead? CC @kuserich @mahdiyazdani

[mahdiyazdani]

Do you think it would make sense to start implementing our own react integration instead?

Have you already started working on a wrapper component that we can use in the “Slider” block and have it initialized with support for reflecting changes as expected? I don’t think we can do much here, as the react library seems to be limiting us in terms of offering a live reflection of changes made to the slider settings in the editor.

Reflecting instant changes associated with options such as page dots and navigation arrow icons would be essential to have in place for the slider component, as otherwise, we would need to add a temporary class name specific to the editor rendering of the block to toggle the visibility of these elements from the view in respect to the value assigned to each control. However, this might not work for settings like "arrow-shape," etc.