Remove color validation in appearance admin page & add color indicator

[dsevillamartin]

Fixes #2121

Changes proposed in this pull request:

• Remove color validation in appearance page
• Add color indicator to inputs

Reviewers should focus on:

• Color validation can be done with dummy HTML elements, but from the issue comments I don't think we want to validate at all.
• After making this I thought this might be a good use for a new component like ColorInput or something? That way it also avoids using ::after for the box... though a component would have to be an entire form group.
  • Perhaps a setting type instead? Though type="color" is also a valid input type.
  • FYI, input::after is not valid so that's why I had to add it to Form-group

Screenshot

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

[SychO9]

I love this!

After making this I thought this might be a good use for a new component like ColorInput or something? That way it also avoids using ::after for the box... though a component would have to be an entire form group.

I think a ColorInput would be perfect, though why would it have to be a Form-group ? if it's about wrapping the input itself, I guess we can just wrap it in a Color-input div and have that within the form group. Would be consistent with Search-input as well.

Perhaps a setting type instead? Though type="color" is also a valid input type.

my experience with type=color (at least on chrome) is that it's not great, it does provide a native color picker which is nice but.. can be a pain to use.

[dsevillamartin]

I think a ColorInput would be perfect, though why would it have to be a Form-group

Nevermind, it doesn't. I thought the CSS stylings were from it lol. But there's also FormControl class.

my experience with type=color (at least on chrome) is that it's not great, it does provide a native color picker which is nice but.. can be a pain to use.

What I meant with mentioning type=color is that I wouldn't want to add an option (like select, bool) to buildSettingComponent that replaces the color type as that is technically a valid HTML input type. I know we don't want to use it but I don't think we would want to replace its uses. Would be potential breaking change as well, I guess.

[SychO9]

What I meant with mentioning type=color is that I wouldn't want to add an option (like select, bool) to buildSettingComponent that replaces the color type as that is technically a valid HTML input type. I know we don't want to use it but I don't think we would want to replace its uses. Would be potential breaking change as well, I guess.

Oh yeah that's a good point. Hmm.. colour no that's a joke..

I think it'd be fine to go with whatever naming, custom-color or something along the lines, as long as we can use it.

[dsevillamartin]

Made it a component instead + the validation.

One can click through both the box & icon. I tested adding a padding so text doesn't go underneath but then you can't click through the box to the input as it's no longer there... and so I decided against it.

My only concern with the code is how it obtains the value currently. I couldn't think of a good way to accomplish that.

[davwheat]

My only concern with the code is how it obtains the value currently. I couldn't think of a good way to accomplish that.

The only other way I can think of is a data attribute, which you can then use with the attr(data-value) function.

[dsevillamartin]

The only other way I can think of is a data attribute, which you can then use with the attr(data-value) function.

Oh, true, but I wasn't referring to the CSS value. I meant the input value from the JS side that is then passed - I don't like using the attrs.bidi?.() || attrs.value as it looks odd and could potentially misrepresent the value at some point? Not sure.

Tried to override oninput but then that'd be on every view render and I didn't know if that was a good idea either (could also be done in initAttrs I guess and then set as a temporary attr when passed?)

[SychO9]

One can click through both the box & icon. I tested adding a padding so text doesn't go underneath but then you can't click through the box to the input as it's no longer there... and so I decided against it.

I think if we wrap it in a label we could still click on the box/icon and have the input be focused, I'll test this later to confirm though.

[davwheat]

Should do, most definitely with pointer-events: none; on the preview and invalid box.

[dsevillamartin]

Should do, most definitely with pointer-events: none; on the preview and invalid box.

They already have this :P

I think if we wrap it in a label we could still click on the box/icon and have the input be focused, I'll test this later to confirm though.

👍

[dsevillamartin]

Re-requested reviews just to clear them 👍

[SychO9]

Looks good!

a nitpick though, the class naming,

as per the convention we follow, Color-input--preview should be Color-input-preview, same for Color-input--icon. I'd also rename Color-input to ColorInput.

[askvortsov1]

Phenomenal, thank you so much! I'm very excited to see continued growth of Flarum's component library :)

Only thing is, we should probably export it in compat so extensions can use it too.

Can't wait till we never have to deal with compat again...