georgealways / lil-gui

Makes a floating panel for controllers on the web. Works as a drop-in replacement for dat.gui in most projects.
https://lil-gui.georgealways.com/
MIT License
1.13k stars 43 forks source link

ColorController changes CSS string format #14

Closed awelles closed 2 years ago

awelles commented 2 years ago

You can see it on the homepage here. Change each of the gui control to a different color and watch the properties on the object: https://lil-gui.georgealways.com/#Guide#Colors

color1 gets changed to lowercase, which is mildly unexpected. The 3 other colors fair worse.

georgealways commented 2 years ago

Yes, that's the intended behavior. The 3 supported color string formats, rgb(,,) #rgb and #rrggbb, all get normalized to #rrggbb when they're written—the assumption being that anything supporting those first two will likely support the third.

If there's a really compelling reason to match the color string format verbatim, then I'm open to it, but right now it seems enough to honor the data type. The language for that section of the guide could probably be more clear.

awelles commented 2 years ago

I threw together a comparison of datgui vs lilgui handling of color properties here. Datgui's a bit more faithful to the original formats.

Honestly I don't have strong feelings regarding the need to handle additional formats.

I do feel weird about a source object's properties changing type and/or format though, since I could be plugging the gui into some object who's source code I don't control.

I just added .inFilter() and .outFilter() to the type-ignorant InputController I wrote to transform data at the point of contact with the object: in getValue/setValue. It shouldn't mess with any functionality if you don't define the filters, but allows weird object properties to be handled pretty easily when needed.

gui.addInput( params, 'date', 'date' )
.inFilter( ( value ) => {
    const date = new Date( value );
    const dString = date.toISOString().split( 'T' )[ 0 ];
    return dString;
} )
.outFilter( ( value ) => {
    const date = new Date( value );
    return date.toLocaleString('default', { month: 'long', day: 'numeric', year: 'numeric' })
} );

How do you feel about something like that on Something like that on the base Controller class? I can open a new issue if you'd like.

georgealways commented 2 years ago

I do feel weird about a source object's properties changing type and/or format though, since I could be plugging the gui into some object who's source code I don't control.

I agree, changing a color property's data type should never happen: is that happening to colors somewhere I'm not seeing?

Let's open another issue if you want for the filter methods, but I don't think that's justified as part of core: I can't think of how the built-in controllers would benefit from it.

awelles commented 2 years ago

I agree, changing a color property's data type should never happen: is that happening to colors somewhere I'm not seeing?

No. typeof property does not change. Just the color string format, as you said, in the case of rgb and rgba color strings.

I don't have a pressing need for those to be preserved.

I did want a way to preserve them when needed. But the // common three.js + dat.gui color pattern example under 'Migrating' pointed the way to accomplish everything I wanted with filter methods without any change to core. So I'm all good here.

Thanks!

georgealways commented 2 years ago

Yes, that's the way forward! Going to keep this open for now in case people come by with the same question.

georgealways commented 2 years ago

Docs were changed a few versions ago to make the string format normalization more explicit.