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

refactor(esl-carousel): complete rework of carousel container & events API #2522

Merged
merged 16 commits into from
Jul 18, 2024

Conversation

ala-n
Copy link
Collaborator

@ala-n ala-n commented Jul 18, 2024

Container Feature Rework Notes

Hey folks (@exadel-inc/esl-core-team),

We had discussed the following implementation for the container class feature:

<esl-carousel 
                media="@XS|@SM|@MD|@LG|@XL"
                count="1|2|3|4|5"
                loop="true"
                esl-carousel-container="{target: '::parent(div)', incompleteCls: 'incomplete'}">
    <ul esl-carousel-slides>
        <li esl-carousel-slide esl-carousel-container-class="some-cls">....</li>
    </ul>
</esl-carousel>

It took a lot of effort and time, but I implemented it. Unfortunately, I was not satisfied with many of the technical results achieved with the current implementation:

  1. Simplicity and Performance (Plugin Size)

    • Contrary to expectations, the implementation of the plugin became complex. It resulted in a full plugin with approximately 200 LOC and additional complications for the user.
  2. User Experience

    • The solution is still not very obvious from a UX perspective.
  3. Missing Information

    • A lot of necessary information was not available in the carousel events.

Alternative Solution (Presented in the Current PR)

Regarding the first two points, I took the risk to make the implementation straightforward and simple with one obvious advantage - it became the responsibility of the carousel itself:

<esl-carousel 
                media="@XS|@SM|@MD|@LG|@XL"
                count="1|2|3|4|5"
                loop="true"
                container="::parent(div)"
                container-empty-class="optional-empty" 
                container-incomplete-class="optional-incomplete">
    <ul esl-carousel-slides>
        <li esl-carousel-slide container-class="some-cls">....</li>
    </ul>
</esl-carousel>

Pros:

  • Around 20 LOC (10 times shorter)
  • Highly performant with no plugins and original hooks
  • Very simple
  • Original API for slides
  • More logical UX:
    • No pre-requirements for the container-class of slides, with clean and predictable behavior
    • Marker classes are optional and easy to provide
    • No future problems with the plugin API. We expect to see plugins potentially dependent on media conditions. It sounds excessive and unnecessary to have that for the container classes feature.

Regarding 3-rd point - it have found -> it got fixed (I hope :) )

@ala-n ala-n requested review from a team, yadamskaya, abarmina and fshovchko and removed request for a team July 18, 2024 02:36
@ala-n ala-n self-assigned this Jul 18, 2024
@ala-n ala-n added the needs review The PR is waiting for review label Jul 18, 2024
@ala-n ala-n added this to the ⚡ESL: 5.0.0 Major release milestone Jul 18, 2024
@NastaLeo
Copy link
Collaborator

For me the second version is okay)

@ala-n ala-n merged commit ef418f1 into main-beta Jul 18, 2024
7 checks passed
@ala-n ala-n deleted the feat/esl-carousel-container branch July 18, 2024 20:58
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 2024
@ala-n
Copy link
Collaborator Author

ala-n commented Jul 22, 2024

🎉 This issue has been resolved in version 5.0.0-beta.25 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs review The PR is waiting for review released on @beta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants