color-js / elements

WIP
14 stars 1 forks source link

[color-chart] First stab at adding `ymin` and `ymax` #134

Closed DmitrySharabin closed 3 weeks ago

DmitrySharabin commented 1 month ago

There's an issue with tooltips that I don't know how to address yet—I'm still experimenting.

netlify[bot] commented 1 month ago

Deploy Preview for color-elements ready!

Name Link
Latest commit d10be9a845e0a278b0be2c45c94efe79ae9498bd
Latest deploy log https://app.netlify.com/sites/color-elements/deploys/6728d3d6a68365000899864c
Deploy Preview https://deploy-preview-134--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.

DmitrySharabin commented 1 month ago

I have an idea for how we can make this change non-breaking: We can apply overflow: hidden only for charts with more than one point out of range.

We can add a private #outOfRange property and set it to false before rendering the chart. Then, if we find any out-of-range point while rendering the underlying color scales, we can set it to true.

Something like this:

...
#outOfRange;

render (evt) {
    ...
    this.#outOfRange = false;
    ...
    for (let colorScale of colorScales) {
        ...
    }
    ...

    if (this.#outOfRange) {
        this._el.chart.style.setProperty("overflow", "hidden");
    }
    else {
        this._el.chart.style.removeProperty("overflow");
    }
}

renderScale (colorScale) {
    ...
    let outOfRange = (isFinite(yMin) && y < yMin) || (isFinite(yMax) && y > yMax);
    if (!outOfRange) {
        // Only swatches that are in range participate in the min/max calculation
        ...
    }
    else {
        this.#outOfRange = true;
    }
    ...
}

Or we can probably set a class.

What do you think?

DmitrySharabin commented 4 weeks ago

OK, I followed @LeaVerou's suggestion to use the Popover API to overcome the overflow: hidden limitations. It also uses the anchor positioning under the hood.

@LeaVerou, could you please take a look at the changes I made? I'll try to point them out for you in the comments.

LeaVerou commented 4 weeks ago

What is the experience if anchor positioning is not supported?

DmitrySharabin commented 4 weeks ago

What is the experience if anchor positioning is not supported?

After experimenting with it (and almost pulling all my hair out of all the browser crashes), I decided to drop the support of anchor positioning in favor of the predictable, manual positioning of the popover.

Could you please give this PR another shot when you have some spare cycles?

DmitrySharabin commented 3 weeks ago

I addressed all your feedback. Thank you!

Is there anything else we should fix before merging this PR?

LeaVerou commented 3 weeks ago

I wonder if we need to separate the specified yMin/yMax from the resolved yMin/yMax. Specifying ymax="120" is very different than 120 just being the max of the range. Right now, there is no way to tell because element.yMax would just return 120. Any precedent from native HTML elements you can think of?

DmitrySharabin commented 3 weeks ago

I wonder if we need to separate the specified yMin/yMax from the resolved yMin/yMax. Specifying ymax="120" is very different than 120 just being the max of the range. Right now, there is no way to tell because element.yMax would just return 120. Any precedent from native HTML elements you can think of?

I think <input type=number /> is one of those elements: We can specify its value with the value attribute (as a string) and, depending on whether it can be transformed to a number or not, the resolved value (which can be accessed via valueAsNumber) will be either a number or NaN.

I agree that separating the specified yMin/yMax from the resolved yMin/yMax makes sense. How about naming the resolved ones yMinAsNumber/yMaxAsNumber to be aligned with the platform?