[BUG] Fixed `isLoading` logic in `EuiConfirmModal` docs #7026

1Copenut · 2023-08-01T23:05:21Z

Summary

Fixed the confirmation modal where users are asked to type delete into an input to enable the red confirm delete button. Closes #7013.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Checked Code Sandbox works for any docs examples

* Updated isLoading prop to check for input string value.

cee-chen · 2023-08-01T23:10:47Z

src-docs/src/views/modal/confirm_modal_loading.tsx

@@ -49,12 +49,12 @@ export default () => {
        buttonColor="danger"
        initialFocus="[name=delete]"
        confirmButtonDisabled={value.toLowerCase() !== 'delete'}
-        isLoading={isLoading}
+        isLoading={value.toLowerCase() !== 'delete' ?? isLoading}


this is a bandaid fix, it doesn't address the root issue above as to why setIsLoading(false) up above does not correctly work.

I'll take another run at it, and focus on the setIsLoading logic.

kibanamachine · 2023-08-01T23:27:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026/

kibanamachine · 2023-08-01T23:30:11Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026_buildkite/

kibanamachine · 2023-08-02T15:27:19Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026_buildkite/

kibanamachine · 2023-08-02T15:30:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026/

cee-chen · 2023-08-02T16:17:21Z

src-docs/src/views/modal/confirm_modal_loading.tsx

-  const searchTimeout = setTimeout(() => {
-    // Simulate a remotely-executed search.
-    setIsLoading(false);
-  }, 1200);
-
-  clearTimeout(searchTimeout);
+  useEffect(() => {
+    if (value === 'delete') {
+      setIsLoading(false);
+    }
+  }, [value]);


Trevor, I'm not sure you're understanding the intention of the demo and its code. :)

Take a look at the demo's description to help understand what the goal of this demo should be. Particularly: "This is helpful to indicate the fetching of data".

With that in mind, it becomes clear that the original setTimeout() was mocking an production async API call that takes 1.2 seconds to complete.

When writing code for demos/examples, we need to be conscious of "does this make sense as a scenario that a consumer might run into? And if so, how can we demonstrate to them the right UX to apply?" In this case, the UX you've written does not make sense. We don't want to represent a loading state when a user hasn't entered in the correct text. A loading state only makes sense for async data that isn't yet available.

Per the demo description again: "wait for a user's input before enabling the confirm action". We want to represent a disabled state when delete has not yet been typed. This logic was already being handled by the previous code:

eui/src-docs/src/views/modal/confirm_modal_loading.tsx

Line 51 in ff296ff

confirmButtonDisabled={value.toLowerCase() !== 'delete'}

So again, let's take this all the way from the top. The setTimeout is not the issue, it's the fact that it's never actually getting called.

kibanamachine · 2023-08-04T17:37:33Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026_buildkite/

kibanamachine · 2023-08-04T17:39:00Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026/

cee-chen · 2023-08-04T17:42:39Z

src-docs/src/views/modal/confirm_modal_loading.tsx

  const closeModal = () => {
    setIsModalVisible(false);
    setIsLoading(false);
-    clearTimeout(searchTimeout);
+    setValue('');


Great catch on this - I really like this addition!

cee-chen · 2023-08-04T17:46:47Z

src-docs/src/views/modal/confirm_modal_loading.tsx

+    setTimeout(() => {
+      // Simulate a remotely-executed search.
+      setIsLoading(false);
+    }, 1200);


I'd be curious to hear your rationale for inlining the setTimeout vs leaving searchTimeout and the clearTimeouts in place. I don't strongly object to it/wouldn't consider it a blocker, but I'd like to hear the reasoning for changing it from the previous code.

I will note it does lead to potential race conditions: if a user opens the modal and then quickly closes it and re-opens it - the setTimeout will still be running in the background and the modal will render on 2nd open as loaded as opposed to loading.

The reason I wouldn't consider this a blocker is because this is a pretty basic demo and doesn't necessarily need to account for such edge cases. However, it seems odd to go from accounting for that behavior in code to no longer accounting for that behavior.

I've been refactoring it after thinking more about. I added a useEffect and this is the code I'm working with locally that properly clears the timeout:

const [isLoading, setIsLoading] = useState(false); const [isModalVisible, setIsModalVisible] = useState(false); const [value, setValue] = useState(''); const showModal = () => { setIsModalVisible(true); setIsLoading(true); }; const closeModal = () => { setIsModalVisible(false); setIsLoading(false); setValue(''); }; useEffect(() => { const searchTimeout = setTimeout(() => { // Simulate a remotely-executed search. setIsLoading(false); }, 1200); return () => { clearTimeout(searchTimeout); }; }, [isModalVisible]); const onChange = (e: React.ChangeEvent<HTMLInputElement>) => { setValue(e.target.value); }; ...

Can you clarify what exactly about the clearTimeout wasn't working? I'm not understanding why simply adding searchTimeout() to the old openModal call wasn't working, and why all this extra logic is needed.

Sure thing. When I loaded the docs site, searchTimeout wouldn't run if I tried to invoke it from inside showModal(). VSCode gave me an error

This expression is not callable. Type 'Timeout' has no call signatures.ts(2349)

Ha I did totally miss the part where searchTimeout isn't actually a function, apologies.

Why not make it as such? Would that not simplify/negate having to use a useEffect? e.g.

let timeoutId; const searchTimeout = () => setTimeout(() => setIsLoading(false), 1200)); const openModal = () => { // ... timeoutId = searchTimeout(); }); const closeModal = () => { // ... clearTimeout(timeoutId); });

I rewrote the solution to use this suggestion. I was having a time figuring out how to declare, then call the setTimeout(). Your suggestion here makes the logic simpler and removes the two run useEffect() which is much cleaner.

kibanamachine · 2023-08-07T14:48:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026_buildkite/

kibanamachine · 2023-08-07T14:49:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026/

src-docs/src/views/modal/confirm_modal_loading.tsx

cee-chen · 2023-08-07T14:59:32Z

src-docs/src/views/modal/confirm_modal_loading.tsx

-  }, 1200);
-
-  clearTimeout(searchTimeout);
+  let timeOutId: ReturnType<typeof setTimeout>;


[not a change request, just a nit] I personally would stick to the casing of the existing API, e.g. timeoutId since setTimeout does not capitalize the out

Co-authored-by: Cee Chen <[email protected]>

cee-chen

🚢

cee-chen

Wait, sorry, your timeout name changes reverted re-adding the comment.

src-docs/src/views/modal/confirm_modal_loading.tsx

cee-chen

To be clear, I wasn't asking for the comment to be restored in exactly the format it was before - above the const searchTimeout was also fine. It just needed to still be there :)

Not a big deal either way, let's get this in

kibanamachine · 2023-08-07T15:52:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026_buildkite/

kibanamachine · 2023-08-07T15:52:52Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7026/

elasticmachine · 2023-08-07T16:13:53Z

💚 Build Succeeded

Buildkite Build
Commit: df44741

History

💚 Build #216 succeeded b50ae20
💚 Build #202 succeeded a05948e
💚 Build #165 succeeded 94dc103
💚 Build #162 succeeded ff296ff

cc @1Copenut

[BUG] Fixed isLoading logic in EuiConfirmModal

ff296ff

* Updated isLoading prop to check for input string value.

1Copenut added bug documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog labels Aug 1, 2023

1Copenut self-assigned this Aug 1, 2023

1Copenut requested review from cee-chen and tkajtoch August 1, 2023 23:06

cee-chen reviewed Aug 1, 2023

View reviewed changes

Refactored away from setTimeout to useEffect.

94dc103

1Copenut requested a review from cee-chen August 2, 2023 15:04

cee-chen reviewed Aug 2, 2023

View reviewed changes

1Copenut added 2 commits August 4, 2023 11:48

Merge branch 'main' into bug/7013-docs-modal-confirm

ff0cd90

Simplified solution to mock async data, then delete flows.

a05948e

1Copenut requested a review from cee-chen August 4, 2023 17:13

cee-chen reviewed Aug 4, 2023

View reviewed changes

1Copenut added 2 commits August 4, 2023 13:52

Adding a proper useEffect to clear timeout.

1a2045c

Refactoring to simpler setTimeout without useEffect.

b50ae20

1Copenut requested a review from cee-chen August 7, 2023 14:27

cee-chen reviewed Aug 7, 2023

View reviewed changes

src-docs/src/views/modal/confirm_modal_loading.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Aug 7, 2023

View reviewed changes

1Copenut and others added 2 commits August 7, 2023 10:10

Update src-docs/src/views/modal/confirm_modal_loading.tsx

a0e550a

Co-authored-by: Cee Chen <[email protected]>

Renaming timeout let for consistency.

52b89c5

cee-chen approved these changes Aug 7, 2023

View reviewed changes

cee-chen requested changes Aug 7, 2023

View reviewed changes

cee-chen reviewed Aug 7, 2023

View reviewed changes

src-docs/src/views/modal/confirm_modal_loading.tsx Outdated Show resolved Hide resolved

Restoring explanatory comment.

df44741

1Copenut requested a review from cee-chen August 7, 2023 15:29

cee-chen approved these changes Aug 7, 2023

View reviewed changes

1Copenut enabled auto-merge (squash) August 7, 2023 15:35

1Copenut merged commit e264226 into elastic:main Aug 7, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Fixed `isLoading` logic in `EuiConfirmModal` docs #7026

[BUG] Fixed `isLoading` logic in `EuiConfirmModal` docs #7026

1Copenut commented Aug 1, 2023 •

edited

Loading

cee-chen Aug 1, 2023 •

edited

Loading

1Copenut Aug 1, 2023

kibanamachine commented Aug 1, 2023

kibanamachine commented Aug 1, 2023

kibanamachine commented Aug 2, 2023

kibanamachine commented Aug 2, 2023

cee-chen Aug 2, 2023 •

edited

Loading

kibanamachine commented Aug 4, 2023

kibanamachine commented Aug 4, 2023

cee-chen Aug 4, 2023

cee-chen Aug 4, 2023 •

edited

Loading

1Copenut Aug 4, 2023

cee-chen Aug 4, 2023 •

edited

Loading

1Copenut Aug 4, 2023

cee-chen Aug 4, 2023 •

edited

Loading

1Copenut Aug 7, 2023

kibanamachine commented Aug 7, 2023

kibanamachine commented Aug 7, 2023

cee-chen Aug 7, 2023

cee-chen left a comment

cee-chen left a comment

cee-chen left a comment

kibanamachine commented Aug 7, 2023

kibanamachine commented Aug 7, 2023

elasticmachine commented Aug 7, 2023

[BUG] Fixed isLoading logic in EuiConfirmModal docs #7026

[BUG] Fixed isLoading logic in EuiConfirmModal docs #7026

Conversation

1Copenut commented Aug 1, 2023 • edited Loading

Summary

QA

General checklist

cee-chen Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

1Copenut Aug 1, 2023

Choose a reason for hiding this comment

kibanamachine commented Aug 1, 2023

kibanamachine commented Aug 1, 2023

kibanamachine commented Aug 2, 2023

kibanamachine commented Aug 2, 2023

cee-chen Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Aug 4, 2023

kibanamachine commented Aug 4, 2023

cee-chen Aug 4, 2023

Choose a reason for hiding this comment

cee-chen Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

1Copenut Aug 4, 2023

Choose a reason for hiding this comment

cee-chen Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

1Copenut Aug 4, 2023

Choose a reason for hiding this comment

cee-chen Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

1Copenut Aug 7, 2023

Choose a reason for hiding this comment

kibanamachine commented Aug 7, 2023

kibanamachine commented Aug 7, 2023

cee-chen Aug 7, 2023

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 7, 2023

kibanamachine commented Aug 7, 2023

elasticmachine commented Aug 7, 2023

💚 Build Succeeded

History

[BUG] Fixed `isLoading` logic in `EuiConfirmModal` docs #7026

[BUG] Fixed `isLoading` logic in `EuiConfirmModal` docs #7026

1Copenut commented Aug 1, 2023 •

edited

Loading

cee-chen Aug 1, 2023 •

edited

Loading

cee-chen Aug 2, 2023 •

edited

Loading

cee-chen Aug 4, 2023 •

edited

Loading

cee-chen Aug 4, 2023 •

edited

Loading

cee-chen Aug 4, 2023 •

edited

Loading