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

[feat] Centralized event-bus to manage synced carousels #94

Closed
ismail9k opened this issue Jun 17, 2019 · 2 comments · Fixed by #95
Closed

[feat] Centralized event-bus to manage synced carousels #94

ismail9k opened this issue Jun 17, 2019 · 2 comments · Fixed by #95
Assignees
Labels
enhancement New feature or request feature

Comments

@ismail9k
Copy link
Contributor

Motivation

The current approach to synchronize two carousels requires that the two carousels must be in the same context, where Hooper search for a carousel with ref prop same as the current carousel's synced prop value and link them together. This approach seemed to have many issues, especially when passing the carousels to another component slot (#69 #55).

Suggested Solution

A new centralized event-bus to manage synced-carousels, which each carousel with synced prop subscribe to, and when one of the subscribed carousels is updated it notifies all the other carousels which have the same synced prop name.

Using Example

<hoooper synced="myCarousel">
    ...
</hooper>

<hoooper synced="myCarousel">
    ...
</hooper>

<hoooper synced="myCarousel">
    ...
</hooper>

Benefits

  • Ability to sync more than two carousels
  • Syncing carousels will be context independent
  • Fix issue when passing carousel to another component slot

Drawbacks

Deprecates the ref/synced API to link two carousels, instead, use synced prop for all carousels with the same value.

Alternatives

N/A

@ismail9k ismail9k added enhancement New feature or request feature labels Jun 17, 2019
@logaretm
Copy link
Contributor

logaretm commented Jun 17, 2019

I suggest we drop the propsynced as it implies boolean value, instead we could go for group name which is clearer. I thought about sync but it would be confusing since prop.sync modifier is a thing.

<hoooper group="myCarousel">
    ...
</hooper>

<hoooper group="myCarousel">
    ...
</hooper>

<hoooper group="myCarousel">
    ...
</hooper>

@ismail9k
Copy link
Contributor Author

Seems better for me, we will deprecate the old props completely, and introduce the more intuitive group prop.

@logaretm logaretm mentioned this issue Jun 18, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants