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

Color picker call events #136

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

luttje
Copy link
Contributor

@luttje luttje commented Mar 8, 2023

I ran across #132 and was inspired to contribute. This PR makes the component forwards the events input (on save) and change (on changing color). Users can listen for them like this:

<x-color-picker name="color" x-on:input="console.log('input', $event.detail)" @change="console.log('change', $event.detail)" />

Warning
This PR builds on my earlier branch from PR #135. This is so I could test on Windows. I can rebase it to the latest release if you want

@driesvints
Copy link
Member

Thanks @luttje. I like this PR but have a couple of thoughts:

I think we should have more descriptive event names so it's clear that it's an event happening for this specific component. I believe both event dispatches can be called the same. What about buk.color_change. This way we also "namespace" it in the Blade UI Kit sphere.

What if there's multiple color pickers? How does the code know which element it came from? I wonder if we can adjust the payload like:

$dispatch('buk.color_change', {
    id: '{{ $id }}',
    color: currentColor
});

@luttje
Copy link
Contributor Author

luttje commented Mar 27, 2024

Heya, thanks for checking out this PR.

You bring up some good points. I didn't take into account (nor test) what would happen with multiple color picker components on the page.

It's been a while since I've worked on this PR and I'm spending some time on other projects atm.

I may have some time to rework this PR this weekend. I'll apply a namespace to event names and send the id along. I'll make a test that has multiple components on 1 page to see if only those respond that should.

A tiny decision I'd like to ask your preference on:
Are you set on event casing using buk.snake_case or are you open to using buk:kebab-case, using colon for namespacing like livewire, e.g: buk:color-change? I personally prefer the hyphens over underscores in HTML since - is more commonly used in HTML (e.g: in class names). If we're mimicing default JS events it could even become buk:colorchange.

Alpine works fine with either, and both have pro's and cons when you start using short-hand syntax and modifiers:

<x-color-picker x-on:buk:color-change.throttle="..." />

<x-color-picker @buk:color-change.throttle="..." />

<!-- or -->

<x-color-picker x-on:buk.color_change.throttle="..." />

<x-color-picker @buk.color_change.throttle="..." />

I can't find any conventions/specs online on how to namespace events (other than most projects using either : or .). It seems to be preference so I'll leave it up to you to choose the styling.

@driesvints
Copy link
Member

buk:color-change sounds good!

@luttje
Copy link
Contributor Author

luttje commented Mar 30, 2024

@driesvints I've implemented the changes we discussed, two things to note:

  1. While I said I'd write a test for multiple components on the page, to see how they'd respond, I haven't done this. This is because the tests are PHP-only I don't see a way to easily test the JavaScript side for now.
  2. While working with $dispatch I realized listening code can get the color picker emitting the event through $event.target (or similar properties on $event like srcElement):
     <div x-data="{}" x-on:buk:color-change="console.log($event.target)">
    Nevertheless I've added $id, since it may be useful for other reasons

Let me know if you want me to change anything else here. I've got time.

Thanks in advance for checking out the PR, and thanks for maintaining this project 👍

@driesvints
Copy link
Member

@luttje thank you so much for these adjustments! Unfortunately there are now some conflicts because I merged a big PR for Alpine v3 support: #156. Could you rebase your PR and solve the conflicts? Best also check if the new event emitting works for the new Alpine version. Thanks! 🙏

@luttje
Copy link
Contributor Author

luttje commented Apr 3, 2024

Sure thing, I'll get on it this maybe next weekend 👍

Edit: didn't find the time, will try again coming weekend (13/14th of april)

@luttje luttje force-pushed the feature/color-picker-event branch from 3a21278 to 3f72128 Compare April 14, 2024 06:04
@luttje
Copy link
Contributor Author

luttje commented Apr 14, 2024

@driesvints Should be rebased and working, ready to squash and merge.

I added some e2e tests that match the alpine event listeners I described in blade-ui-kit/docs#22. That should give some certainty that it works with the latest alpine.

Let me know if there's anything else, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

How do I run an AlpineJS function when a new x-color-picker color is selected?
2 participants