symfony
[Live] Add `data-error` attribute + fix loading behaviour on error
| 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
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")
@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 !
Thanks @WebMamba !
I just tried it, and it does work for me, the class is added on the element with the attribute.
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.
Wherer are we on this one ? are you blocked by something ? Can i help you in any way @YummYume ?
Hey @smnandre ! I'll work on resolving the conflicts ASAP but other than that, no, I think it was all good !
If there are any suggestions or changes wanted to the current behavior, I'm all ears.
A couple of questions / comments, but to me we're almost there 😄
I'm still a bit doubtfull we need all the modifier and/or should maybe refactor the code avoid doubling the code with the existing ones, but this (refactor the existing) could be done in a later step / PR.
Do we really need the showError method for instance ? I mean do you think all the JS code can be simplified, more "direct" ? (genuine question)
Also, i'm not a big fan of the added CSS and would love for use to remove it entirely in future versions, so is there any way to handle this feature without adding CSS ?
Rebase is done.
Some of the code could perhaps be unified, but there was already a big part done (all the parsing is already shared between the loading and error plugins). I also made sure not to include too complex behaviors, such as targeting a specific action (which makes no sense here) or using modifiers with the attributes.
So with all that, I don't see a reason why not include them ? At least it stays consistent with the loading plugin. I did my best to make the plugin readable and easy to understand for any further modification, especially since this PR is technically only the first part of our long discussion on how to properly handle errors.
I agree with you for the CSS. On top of that, it doesn't always work. There can still a short window where you can actually see the element, until this style is loaded. Which is why (and I added this part in the doc as well), I always add my own display: none; class on my data-loading or data-live-error elements. I think the best solution would simply to remove the live.css stylesheet and warn the user about the need to add their own hidden class to their elements.
Thank you for the quick answers, make sense!
Let's update the src/LiveComponent CHANGELOG (it's gonna be 2.20)
@kbond @WebMamba @Kocal if one of you can give a second check ?
I am closing due to inactivity, feel free to re-open the PR if necessary. Thanks for the contribution!
