Personally, I feel like using a toast here is not the right UI pattern. Normally, input validation errors are shown right below the input so that it is clear which input the error is shown for.

Just to make it clear, I don't think this should be a blocking comment but something worth thinking about.

cc @ellaestigoy

Instead of having to manually validate this, I think we can just rely on the minLength and maxLength attributes on the input to restrict the length?

Also, it has limitation that toast will show only when admin hit enter (13 keyCode). But I can click outside and will never know that something is wrong.

Demo

When I am out of focus and value is invalid, should we display error as well?

I don't believe minlength and maxlength would work here. We want to allow values that are either exactly 3 OR 6 chars long; 4 and 5 are illegal.

@lis2 I've pushed a change to show an error toast if the value is invalid when moving focus outside the text input (and show red border around inputs containing invalid colors).

Having to handle all these events manually feels odd to me. In my mind, the pattern should be that each input tracks its own "value" and reacts accordingly when the value of its input changes.

I think this is related to the toasts comment above. I'm using toasts because we don't have an outlet/space in the palette editor to show errors for individual colors. If we change our design to make space for errors, I think we'll be able to use the "tracking" pattern you describe here.

I see. Let's defer this but we should definitely follow up with product/design about this.

I'm curious why we need to set this attribute as it doesn't seem to be used anywhere.

We serialize this attribute in the Theme model. When a theme's color palette is edited via the /admin/themes/:id/change-colors endpoint, it may create a new color palette record and serialize the new record in the response. In that case, the Theme object in the client app should have the newly created record to avoid stale state when/if the Theme object in question is passed to other areas of the app.

I don't seem to see owned_color_palette being eager loaded anywhere so this will result in an N+1 queries problem when this serializer is used to render a list of themes.

owned_color_palette is an alias for owned_color_scheme. Theme#include_basic_relations does include the owned_color_scheme relation.

(having said that, I think this serializer should never be used to render a list of themes --- it sends way too much data for any list. In the new themes and components listing pages, we have smaller serializers that include only the data needed for these respective pages)

I see thank you for clarifying 👍

(having said that, I think this serializer should never be used to render a list of themes --- it sends way too much data for any list. In the new themes and components listing pages, we have smaller serializers that include only the data needed for these respective pages)

I see so does that means this controller action will eventually be removed?

It should be removed given that we no longer call it in the app (/admin/customize/themes has been removed by @lis2), but API compatibility makes it hard to remove endpoints.

Also, it has limitation that toast will show only when admin hit enter (13 keyCode). But I can click outside and will never know that something is wrong.

Demo

When I am out of focus and value is invalid, should we display error as well?

I noticed a bug when I played with it. When I change color and try to leave the page, I see a warning. However, if I frist click settings, warning is not showing. Recorded that

I'm actually not sure if we should the warning if the user goes to the settings tab of the theme. If you go back to the colors tab, the unsaved changes will not be lost, so you can continue editing and save them even if you flick to the settings tab and back. I can certainly show the warning when navigating to the settings tab if you think it makes more sense to show it.