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

YummYume · 2024-06-12T23:03:17Z

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 :

before_pr-2024-06-10_22.31.29.mp4

After :

after_pr-2024-06-11_09.33.28.mp4

smnandre · 2024-06-14T07:21:54Z

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 · 2024-06-14T20:05:42Z

src/LiveComponent/assets/dist/Component/index.d.ts

+    'loading.state:started': (element: HTMLElement, request: BackendRequest) => MaybePromise;
+    'loading.state:finished': (element: HTMLElement) => MaybePromise;


Took me some time to understand this ... as you have some CS changes mixed in here :)

Could you -if you have time- open a PR just for this "bug fix" ?

I know it's only TS and not really a bug, but people will probably see these values in their IDE... and i'd really like them to see the good ones instead 😅

And also to ensure we merge the fix asap .... because this one will depend on the path we choose to take :)

Will do! I just added the typing to the hooks to not have to remember the event names and having a better DX, but I was wondering why my event wasn't firing... Only to find out there was a typo in the typing 😅

YummYume · 2024-06-15T19:03:13Z

@smnandre Thanks for the feedback! 😄 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 !

WebMamba

Wohaaaa! Thanks @YummYume for your PR! It's so cool! I love the path taking here!

I noticed that the data-live-error="addClass(text-red-500)" does not work. I didn't dig to see why but here is my template :

<div {{ attributes }}>
    <p data-live-error="addClass(text-red-500)">Hello</p>
    <button
            data-action="live#action"
            data-live-action-param="error"
    >
        Throw
    </button>
    <span data-live-error="show">An error occurred ! Please try again.</span>
</div>

the class text-red-500 is not added but the span is well added. Maybe you could have a clue.

I think we should disable the pop up we the error by default in production, and actually even in dev ? Do you think this is still needed since we have the request in the profiler ?

YummYume · 2024-06-19T20:48:40Z

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.

This PR was merged into the 2.x branch. Discussion ---------- [Live] Fix typing for loading hooks + HookManager | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Issues | | License | MIT See #1916 (comment) for context Fixes the typo on the loading hooks and add the hook typing to the `HookManager` (was forgotten in my first pr) Commits ------- 9428547 Fix typing for loading hooks + HookManager

This PR was merged into the 2.x branch. Discussion ---------- [Live] Fix typing for loading hooks + HookManager | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Issues | | License | MIT See symfony/ux#1916 (comment) for context Fixes the typo on the loading hooks and add the hook typing to the `HookManager` (was forgotten in my first pr) Commits ------- 94285477 Fix typing for loading hooks + HookManager

smnandre · 2024-09-14T21:43:18Z

Wherer are we on this one ? are you blocked by something ? Can i help you in any way @YummYume ?

YummYume · 2024-09-16T07:26:48Z

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.

…tes removal on error

…rder

smnandre · 2024-09-16T18:43:46Z

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 ?

YummYume · 2024-09-16T19:17:36Z

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.

smnandre · 2024-09-16T19:23:54Z

src/Dropzone/assets/src/style.css

@@ -60,7 +60,7 @@
 }

 .dropzone-preview-button::before {
-    content: '×';
+    content: "×";


Let's remove this :)

We'll make the change in another small PR

I never made that change, I think that's just the linter ?

smnandre · 2024-09-16T19:24:03Z

src/Dropzone/assets/dist/style.min.css

@@ -1 +1 @@
-.dropzone-container{position:relative;display:flex;min-height:100px;border:2px dashed #bbb;align-items:center;padding:20px 10px}.dropzone-input{position:absolute;display:block;top:0;left:0;width:100%;height:100%;opacity:0;cursor:pointer;z-index:1}.dropzone-preview{display:flex;align-items:center;max-width:100%}.dropzone-preview-image{flex-basis:0;min-width:50px;max-width:50px;height:50px;margin-right:10px;background-size:contain;background-position:50% 50%;background-repeat:no-repeat}.dropzone-preview-filename{word-wrap:anywhere}.dropzone-preview-button{position:absolute;top:0;right:0;z-index:1;border:none;margin:0;padding:0;width:auto;overflow:visible;background:0 0;color:inherit;font:inherit;line-height:normal;-webkit-font-smoothing:inherit;-moz-osx-font-smoothing:inherit;-webkit-appearance:none}.dropzone-preview-button::before{content:'×';padding:3px 7px;cursor:pointer}.dropzone-placeholder{flex-grow:1;text-align:center;color:#999}


smnandre · 2024-09-16T19:38:00Z

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 ?

Kocal · 2024-09-17T11:37:56Z

src/LiveComponent/assets/src/Component/plugins/ErrorPlugin.ts

+    private showElement(element: HTMLElement | SVGElement) {
+        element.style.display = 'revert';
+    }
+
+    private hideElement(element: HTMLElement | SVGElement) {
+        element.style.display = 'none';
+    }


If the dev used style="display: flex" on element, what will happens when calling showElement?

Can we add/remove a custom CSS class instead, like data-live-hidden? It would be safer than manipulating element.style.display IMO.

Well, that sadly doesn't work, but this is a current issue with the data-loading attribute as well (see #102).
A simple workaround is to do something like class="hidden" data-live-error="addClass(flex) removeClass(hidden)" instead. That will work.

I also don't think adding more custom CSS would be a good idea, as discussed above, and we would probably have to use !important any way to avoid the style not taking effect.

Kocal · 2024-09-17T11:38:39Z

src/LiveComponent/assets/src/Component/plugins/ErrorPlugin.ts

+
+        if (element.classList.length === 0) {
+            element.removeAttribute('class');
+        }


I think this is not necessary

In fact i believe it is, or the next morph will consider this a change .. or let's say i think i remember this

I'm not sure if this behavior changed with the switch to Idiomorph though

src/LiveComponent/doc/index.rst

carsonbot added Bug Bug Fix Feature New Feature Status: Needs Review Needs to be reviewed labels Jun 12, 2024

smnandre mentioned this pull request Jun 14, 2024

[LiveComponent] Allow disabling the error modal in production #1891

Open

smnandre reviewed Jun 14, 2024

View reviewed changes

smnandre added the LiveComponent label Jun 14, 2024

YummYume force-pushed the feature/error-plugin branch from 7f8b505 to ae2cf69 Compare June 15, 2024 19:28

YummYume mentioned this pull request Jun 15, 2024

[Live] Fix typing for loading hooks + HookManager #1921

Merged

YummYume changed the title ~~Add data-error attribute + fix loading behaviour on error~~ [Live] Add data-error attribute + fix loading behaviour on error Jun 15, 2024

WebMamba reviewed Jun 18, 2024

View reviewed changes

YummYume added 9 commits September 16, 2024 20:35

Added new ErrorPlugin for data-error attributes and fixed loading sta…

c8b8160

…tes removal on error

Perform pending requests even on error

d68caab

Added tests and documentation for data-error

e6038e5

Removed unused css rule for data-error + better documentation

baac4dd

Patch doc lint for data-error

f824a23

Patch typo in data-error doc example

22e36c1

Rename data-error to data-live-error and switch error documentation o…

d349edc

…rder

Revert typing fix for hooks (done in another pr)

ce503ff

Trigger response:error hook after loading.state:finished hook

468ae87

Rebase error plugin + linter

81f01c3

YummYume force-pushed the feature/error-plugin branch from 1ae59fe to 81f01c3 Compare September 16, 2024 18:58

Revert unwanted change for HookManager

8dedb08

Lint HookManager + rebuild assets

876e282

smnandre reviewed Sep 16, 2024

View reviewed changes

Kocal reviewed Sep 17, 2024

View reviewed changes

Fix example typo for live components documentation

cd3121b

smnandre mentioned this pull request Oct 17, 2024

[LiveComponent] Response 400 after application idle for long time #2278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

YummYume commented Jun 12, 2024

smnandre commented Jun 14, 2024

smnandre Jun 14, 2024

YummYume Jun 15, 2024

YummYume commented Jun 15, 2024

WebMamba left a comment

YummYume commented Jun 19, 2024

smnandre commented Sep 14, 2024

YummYume commented Sep 16, 2024

smnandre commented Sep 16, 2024

YummYume commented Sep 16, 2024

smnandre Sep 16, 2024

YummYume Sep 16, 2024

smnandre Sep 16, 2024

smnandre commented Sep 16, 2024

Kocal Sep 17, 2024

YummYume Sep 18, 2024

Kocal Sep 17, 2024

smnandre Sep 17, 2024

YummYume Sep 18, 2024

		'loading.state:started': (element: HTMLElement, request: BackendRequest) => MaybePromise;
		'loading.state:finished': (element: HTMLElement) => MaybePromise;

		@@ -1 +1 @@
		.dropzone-container{position:relative;display:flex;min-height:100px;border:2px dashed #bbb;align-items:center;padding:20px 10px}.dropzone-input{position:absolute;display:block;top:0;left:0;width:100%;height:100%;opacity:0;cursor:pointer;z-index:1}.dropzone-preview{display:flex;align-items:center;max-width:100%}.dropzone-preview-image{flex-basis:0;min-width:50px;max-width:50px;height:50px;margin-right:10px;background-size:contain;background-position:50% 50%;background-repeat:no-repeat}.dropzone-preview-filename{word-wrap:anywhere}.dropzone-preview-button{position:absolute;top:0;right:0;z-index:1;border:none;margin:0;padding:0;width:auto;overflow:visible;background:0 0;color:inherit;font:inherit;line-height:normal;-webkit-font-smoothing:inherit;-moz-osx-font-smoothing:inherit;-webkit-appearance:none}.dropzone-preview-button::before{content:'×';padding:3px 7px;cursor:pointer}.dropzone-placeholder{flex-grow:1;text-align:center;color:#999}

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

Are you sure you want to change the base?

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

Conversation

YummYume commented Jun 12, 2024

smnandre commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YummYume commented Jun 15, 2024

WebMamba left a comment

Choose a reason for hiding this comment

YummYume commented Jun 19, 2024

smnandre commented Sep 14, 2024

YummYume commented Sep 16, 2024

smnandre commented Sep 16, 2024

YummYume commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smnandre commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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