color-js / elements

WIP
13 stars 1 forks source link

First stub on making charts reactive #115

Closed DmitrySharabin closed 4 weeks ago

DmitrySharabin commented 4 weeks ago

Changes:

  1. <color-swatch> — Update color info on every color change
  2. <color-scale> — Expose the colorschange event
  3. <color-chart> — Handle the colorschange event from the underlying color scales
  4. <color-chart> — On the colors set, make sure the last swatch is not connected to the non-existent next swatch

Do we need to propagate the colorschange event so that it can be listened to on <color-chart>?

netlify[bot] commented 4 weeks ago

Deploy Preview for color-elements ready!

Name Link
Latest commit cfa03555a1ecac112c1aa7732ea5089ad1e1dd5c
Latest deploy log https://app.netlify.com/sites/color-elements/deploys/6706e91e820ab7000882c8b8
Deploy Preview https://deploy-preview-115--color-elements.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

LeaVerou commented 4 weeks ago

Nice work!

Do we need to propagate the colorschange event so that it can be listened to on <color-chart>?

Not sure what you mean, but yes, people should definitely be able to listen to that on <color-chart>. But I'd expect that to be the case already?

DmitrySharabin commented 4 weeks ago

Do we need to propagate the colorschange event so that it can be listened to on <color-chart>?

Not sure what you mean, but yes, people should definitely be able to listen to that on <color-chart>. But I'd expect that to be the case already?

I would like to say “Yes,” but it's not. We don't expose any events for <color-chart> (yet?), so we need to dispatch the colorschange event after we handle it internally the way we do it inside some other color elements:

handleEvent (evt) {
    if (evt.target.tagName === "COLOR-SCALE" && evt.name === "colors") {
        this.render(evt);
+       this.dispatchEvent(new evt.constructor(evt.type, {...evt}));
    }
}

I can commit it and merge the PR if you are OK with this change.

LeaVerou commented 4 weeks ago

Do we need to propagate the colorschange event so that it can be listened to on <color-chart>?

Not sure what you mean, but yes, people should definitely be able to listen to that on <color-chart>. But I'd expect that to be the case already?

I would like to say “Yes,” but it's not. We don't expose any events for <color-chart> (yet?), so we need to dispatch the colorschange event after we handle it internally the way we do it inside some other color elements:

handleEvent (evt) {
  if (evt.target.tagName === "COLOR-SCALE" && evt.name === "colors") {
      this.render(evt);
+     this.dispatchEvent(new evt.constructor(evt.type, {...evt}));
  }
}

I can commit it and merge the PR if you are OK with this change.

We don't dispatch it, but since <color-scale> is a child of <color-chart> doesn't it bubble anyway?

DmitrySharabin commented 4 weeks ago

Do we need to propagate the colorschange event so that it can be listened to on <color-chart>?

Not sure what you mean, but yes, people should definitely be able to listen to that on <color-chart>. But I'd expect that to be the case already?

I would like to say “Yes,” but it's not. We don't expose any events for <color-chart> (yet?), so we need to dispatch the colorschange event after we handle it internally the way we do it inside some other color elements:

handleEvent (evt) {
    if (evt.target.tagName === "COLOR-SCALE" && evt.name === "colors") {
        this.render(evt);
+       this.dispatchEvent(new evt.constructor(evt.type, {...evt}));
    }
}

I can commit it and merge the PR if you are OK with this change.

We don't dispatch it, but since <color-scale> is a child of <color-chart> doesn't it bubble anyway?

It doesn't. colorschange is an instance of PropChangeEvent. When we define and fire it, we don't set the bubbles event property, so it's false. I wonder if it would be helpful if we could specify custom options when defining events, like so:

static events = {
    colorschange: {
        propchange: "computedColors",
        bubbles: true,
        cancelable: true,
    },
};

Or like so:

static events = {
    colorschange: {
        propchange: "computedColors",
        options: {
            bubbles: true,
            cancellable: true,
        },
    },
};

Alternatively, we can add these (or any other) options to all PropChangeEvents as sensible defaults. In that case, does it make sense to allow the re-defining of these properties on an event-by-event basis (if authors can use preventDefault() and/or stopPropagation() when needed)?

LeaVerou commented 4 weeks ago

I think setting event options in the dictionary makes perfect sense. I'd probably go for the flat structure, but no strong opinion.

DmitrySharabin commented 4 weeks ago

I think setting event options in the dictionary makes perfect sense. I'd probably go for the flat structure, but no strong opinion.

I would also go for the flat structure: Specifying the prop causing the event to fire is already one of the event options (to my mind), so there is no need for another event option called options.