Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSBreakpointModule: add document.readyState checks #5495

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented May 5, 2024

Closes #5482

@stsrki stsrki requested a review from David-Moreira May 5, 2024 13:11
Copy link
Contributor

@David-Moreira David-Moreira left a comment

Choose a reason for hiding this comment

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

Looks ok, see comment for making sure the event is effectively attached? Also were you able to reproduce the issue? Or just trying to make the code more resilient?

Source/Blazorise/wwwroot/breakpoint.js Outdated Show resolved Hide resolved
@stsrki
Copy link
Collaborator Author

stsrki commented Sep 20, 2024

@David-Moreira can you take care of this PR?

@stsrki
Copy link
Collaborator Author

stsrki commented Sep 20, 2024

PS. I will rebase it to 1.6.

@stsrki stsrki changed the base branch from rel-1.5 to rel-1.6 September 20, 2024 07:59
@David-Moreira
Copy link
Contributor

@David-Moreira can you take care of this PR?

Wasn't this ready? What exactly do you want me to do here?
Do you want to patch 1.6 with this? I don't think I've seen many complaints about this? I'd probably do it for 1.7, what do you think?

@stsrki
Copy link
Collaborator Author

stsrki commented Sep 27, 2024

@David-Moreira can you take care of this PR?

Wasn't this ready? What exactly do you want me to do here? Do you want to patch 1.6 with this? I don't think I've seen many complaints about this? I'd probably do it for 1.7, what do you think?

I thought that you could finish the task. You had some comments previously about module loading and ready states, so maybe you could do it better.

@stsrki
Copy link
Collaborator Author

stsrki commented Sep 27, 2024

@David-Moreira can you take care of this PR?

Wasn't this ready? What exactly do you want me to do here? Do you want to patch 1.6 with this? I don't think I've seen many complaints about this? I'd probably do it for 1.7, what do you think?

I thought that you could finish the task. You had some comments previously about module loading and ready states, so maybe you could do it better.

PS. 1.7 sounds good to me.

@David-Moreira David-Moreira changed the base branch from rel-1.6 to master September 28, 2024 15:10
@David-Moreira
Copy link
Contributor

@stsrki
I guess at the point the module is called from .NET it's unlikely the DOM isn't loaded already. So my concern was probably not warranted.

Anyway:

  • I added a check whether the ready state is in loading, go through the DOMContentLoaded
  • Slightly refactored to exclude redundant comments.

@stsrki stsrki merged commit 21110bb into master Oct 17, 2024
2 checks passed
@stsrki stsrki deleted the rel-1.5-jsbreakpoint-checks branch October 17, 2024 10:42
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: JSBreakpointModule throwing exceptions
2 participants