Skip to content

Added DOM_OBJECTS_UPDATED notification when the DOM is re-rendered via updateDom#3535

Merged
khassel merged 2 commits intoMagicMirrorOrg:developfrom
ryan-d-williams:#3534
Sep 18, 2024
Merged

Added DOM_OBJECTS_UPDATED notification when the DOM is re-rendered via updateDom#3535
khassel merged 2 commits intoMagicMirrorOrg:developfrom
ryan-d-williams:#3534

Conversation

@ryan-d-williams
Copy link
Contributor

@ryan-d-williams ryan-d-williams commented Sep 13, 2024

  • Base your pull requests against the develop branch.
  • Include these infos in the description:
  • What does the pull request accomplish? Use a list if needed.
  • Adds a new notification (DOM_OBJECTS_UPDATED) when the DOM is updated via updateDom
  • Please run npm run lint:prettier before submitting
  • Don't forget to add an entry about your changes to the CHANGELOG.md file.

More info can be found in #3534, but as a summary:

The updateDom function is not synchronous - there is an undetermined amount of time between when it completes and when the DOM has actually been re-rendered and is ready for interaction. The existing notification (MODULE_DOM_CREATED) only fires once on the initial DOM render. This PR solves the issue of subsequent re-renders by adding a new notification that fires whenever the DOM is ready after an update. This notification falls within expected lifecycle notifications (very similar to other libraries that provide DOM lifecycle notifications).

@ryan-d-williams
Copy link
Contributor Author

ryan-d-williams commented Sep 13, 2024 •
edited
Loading

Failed tests seem to be a result of #3532 (PR #3533) and not caused by the PR

@khassel
Copy link
Collaborator

khassel commented Sep 13, 2024

Failed tests seem to be a result of #3532 (PR #3533) and not caused by the PR

yes ...

@ryan-d-williams
Copy link
Contributor Author

ryan-d-williams commented Sep 13, 2024

@khassel sorry I should've phrased my question better:

Do I need to update my PR to have the same changes as #3533 so the tests pass, or can my PR still be accepted with failing tests given they are fixed in #3533?

@khassel
Copy link
Collaborator

khassel commented Sep 13, 2024

or can my PR still be accepted with failing tests

in this case, yes.

@khassel
Copy link
Collaborator

khassel commented Sep 13, 2024

@ryan-d-williams the test fix is ​​merged, best approach would be to rebase/merge your PR on new develop, thanks!

@ryan-d-williams
Copy link
Contributor Author

ryan-d-williams commented Sep 13, 2024

@khassel is it possible to have an admin restart the tests? If develop was updated a restart should show the tests pass?

I'm happy to do a rebase/merge, but that's going to take me a minute of fumbling around and probably destroying my branch at least a few times so if we can take the path of least resistance of restarting the tests I would appreciate it. If that's not possible I'll try to get the branch updated.

@khassel
Copy link
Collaborator

khassel commented Sep 13, 2024

is it possible to have an admin restart the tests?

yes, but they will run on the same commit and fail again

if it is o.k. for you I can do the rebase

@ryan-d-williams @khassel
Added DOM_OBJECTS_UPDATED notification each time the DOM is re-render…
cf3407c 
…ed. Ensures the module can know when the DOM is available for interaction. Fixes MagicMirrorOrg#3534.
@ryan-d-williams
Copy link
Contributor Author

ryan-d-williams commented Sep 13, 2024

@khassel that would be great thank you! If you don't mind letting me know the steps you took - it's been a while since I've done a rebase like this so it would be nice to refresh my memory on it for next time.

@khassel
Copy link
Collaborator

khassel commented Sep 13, 2024

If you don't mind letting me know the steps you took

well, I'm using a graphical tool called "Git Extensions" under Windows, steps:

  • add your repo as remote and fetch it
  • checkout your branch #3534
  • rebase this branch on develop by selecting the target commit
  • force-push the updated branch #3534 (which is only possible for maintainers of the project or you as owner)

@khassel khassel requested review from rejas and sdetweil September 13, 2024 22:36
@ryan-d-williams
Copy link
Contributor Author

ryan-d-williams commented Sep 14, 2024

@khassel thank you! I made it to the end, but when I saw I had to force-push I figured I had messed something up.

Thanks again!

rejas
rejas reviewed Sep 15, 2024
View reviewed changes
@khassel khassel merged commit c6e05c9 into MagicMirrorOrg:develop Sep 18, 2024
KristjanESPERANTO
KristjanESPERANTO reviewed Sep 18, 2024
View reviewed changes
@sdetweil
Copy link
Collaborator

sdetweil commented Sep 18, 2024

i think it should be fixed now in dev.. he just put in in current release.. don't need extra debt..

@khassel
Copy link
Collaborator

khassel commented Sep 18, 2024

so I will fix it on develop ...

@khassel
Copy link
Collaborator

khassel commented Sep 18, 2024

done.

@ryan-d-williams
Copy link
Contributor Author

ryan-d-williams commented Sep 18, 2024

My fault, sorry about that. Thanks for the fix @khassel

@khassel
Copy link
Collaborator

khassel commented Sep 18, 2024

No problem, it's not the first time and I hate the current construction with the changelog ...

This was referenced Sep 19, 2024
Closed
Merged
Closed
rejas pushed a commit that referenced this pull request Sep 19, 2024
@ryan-d-williams
Updated to new notification name DOM_OBJECTS_UPDATED -> `MODULE_DOM…
1823f5a 
…_UPDATED` (#3548)

This is an update to #3535. See #3534 for discussion and context. Fixes
#3534 (again).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khassel khassel khassel left review comments

@KristjanESPERANTO KristjanESPERANTO KristjanESPERANTO left review comments

@sdetweil sdetweil Awaiting requested review from sdetweil

@rejas rejas Awaiting requested review from rejas

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ryan-d-williams @khassel @sdetweil @rejas @KristjanESPERANTO