search

symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
781 stars 274 forks source link

[Live] Add `data-error` attribute + fix loading behaviour on error #1916

Open YummYume opened 2 weeks ago

YummYume commented 2 weeks ago
Q A
Bug fix? yes
New feature? yes
Issues #1891
License MIT

This PR adds a new LoadingPlugin to live components. It allows the use of a data-error that works almost like the data-loading attribute, but for errors. It also fixes the loading state not clearing when an error occurs, and a typo in the hook names in the types.

Little demo of the before/after of this PR :

Before :

https://github.com/symfony/ux/assets/61044908/64d4f70c-773c-4279-88ee-a39662ecd97c

After :

https://github.com/symfony/ux/assets/61044908/f729ae13-79f2-40e4-93c2-d40361c9e652

smnandre commented 2 weeks ago

Thank you very much @YummYume ! I'll review in depth during week-end

A first comment though: i'm not sure we should expose the same API / possibilities, as such errors "should not" happen and we don't want to maintain/document too many things if they are not used.

So maybe could we select only one or two methods ? What do you think ?

Regarding the attribute, data-error maybe is a bit too common, especially with the added CSS. Maybe data-live-error ?

(The documentation seems great, i think i'd inverse the 2 sections and start with the "what" before the "how")

YummYume commented 2 weeks ago

@smnandre Thanks for the feedback! :smile: By that, you mean only allowing the show|hide directives ? I currently implemented the same directives as the data-loading attribute because they were quite easy to add, but didn't add support for any modifier (like delay, models, and such...) as it wouldn't make much sense for an error state.

And I thought the same about the data-error attribute, I just wasn't sure if I should try to align its name with the data-loading one (since they are very similar). I will change it. Good point for the documentation as well !

YummYume commented 1 week ago

Thanks @WebMamba !

I just tried it, and it does work for me, the class is added on the element with the attribute.

image

Is it maybe just that the class does nothing because you're not using Tailwind ?

And about the error modal, I'm not sure, I think it's still a great and more direct debugging tool than the profiler. But for more in depth debugging, it's of course not ideal.