Skip to content

Fix useEffect web component warnings#674

Merged
magdalenajadach merged 24 commits intomainfrom
issues/668-Fix_useEffect_web_component_warnings
Oct 12, 2023
Merged

Fix useEffect web component warnings#674
magdalenajadach merged 24 commits intomainfrom
issues/668-Fix_useEffect_web_component_warnings

Conversation

@create-issue-branch
Copy link
Contributor

closes #668

@create-issue-branch create-issue-branch bot temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings September 28, 2023 09:18 Inactive
@github-actions
Copy link

@github-actions
Copy link

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 3, 2023 10:09 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Magdalena Jadach added 2 commits October 3, 2023 11:17
…hub.com:RaspberryPiFoundation/editor-ui into issues/668-Fix_useEffect_web_component_warnings
@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 3, 2023 10:17 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 3, 2023 10:24 — with GitHub Actions Inactive
@magdalenajadach magdalenajadach marked this pull request as ready for review October 3, 2023 10:24
@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 3, 2023 10:27 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

}, 2000);
setTimeoutId(id);
}, [project]);
}, [project, timeoutId, webComponent]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this doesn't make the browser go crazy? The reason the timeoutId was left out initially was because it was causing constant rerenders (which was throwing up a warning in the console)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will run it locally and see the console, thanks a lot for flagging it up

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loiswells97 I wrapped the method to clear any existing timeout when the dependencies change. Could you please check the console if there are any issues? I couldn't replicate any console issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I took a look and I'm not sure the custom event is still being triggered. For example, if you go to the web component test page it should print the code to the console 2 seconds after it's changed (in response to the codeChanged event), but that isn't happening anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loiswells97 I removed the reset of timeoutId. I wonder if adding a timeout clearing function (clearing interval) might be a good idea to include the timeoutId within the useEffect() dependencies and also make sure it's not causing any console issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still behaving strangely because it now triggers the event multiple times (I think once for each key press) whereas it should only trigger once 2 seconds after the user stops typing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On your point, I'm not sure, I feel like my understanding of the timeout stuff is sketchy at best

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 3, 2023 11:05 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

@magdalenajadach magdalenajadach self-assigned this Oct 3, 2023
@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 3, 2023 12:09 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 9, 2023 12:57 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 10, 2023 10:44 — with GitHub Actions Inactive
@github-actions
Copy link

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 10, 2023 11:40 — with GitHub Actions Inactive
@github-actions
Copy link

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 10, 2023 13:45 — with GitHub Actions Inactive
@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 10, 2023 13:46 — with GitHub Actions Inactive
@github-actions
Copy link

const project = useSelector((state) => state.editor.project);
const codeRunTriggered = useSelector(
(state) => state.editor.codeRunTriggered,
(state) => state.editor.codeRunTriggered
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might have broken the linting?

CHANGELOG.md Outdated

## Unreleased

## [0.19.2] - 2023-10-10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We normally only add this line when we cut a release, and leave all the current stuff under Unreleased

@github-actions
Copy link

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 10, 2023 14:16 — with GitHub Actions Inactive
@github-actions
Copy link

loiswells97
loiswells97 previously approved these changes Oct 10, 2023
Copy link
Contributor

@loiswells97 loiswells97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🎉

@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 10, 2023 15:17 — with GitHub Actions Inactive
@magdalenajadach magdalenajadach temporarily deployed to previews/issues/668-Fix_useEffect_web_component_warnings October 11, 2023 20:18 — with GitHub Actions Inactive
@github-actions
Copy link

Magdalena Jadach added 2 commits October 11, 2023 21:22
…hub.com:RaspberryPiFoundation/editor-ui into issues/668-Fix_useEffect_web_component_warnings
@github-actions
Copy link

Copy link
Contributor

@loiswells97 loiswells97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

@magdalenajadach magdalenajadach merged commit e61900a into main Oct 12, 2023
@magdalenajadach magdalenajadach deleted the issues/668-Fix_useEffect_web_component_warnings branch October 12, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix useEffect web component warnings

2 participants