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

Small typo in sample/resizeObserverHDDPI/index.html #482

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

jomiq
Copy link
Contributor

@jomiq jomiq commented Dec 11, 2024

gscript -> script

@kainino0x kainino0x requested a review from greggman December 12, 2024 04:45
@kainino0x
Copy link
Collaborator

@greggman was this script intentionally disabled?

Copy link
Collaborator

@greggman greggman left a comment

Choose a reason for hiding this comment

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

I think it's fine. I tested locally on Chrome/Firefox/Safari with it set to <script> and it seemed to not have an issues

OTOH, if I add some text to the HTML as in

    <div id="container" class="fit-container">
      <canvas></canvas>
ddd
    </div>

It fails (it starts expanding the canvas indefinitely). The issue is the size the canvas 100% and if the page isn't setup correctly it will make the page larger.

In any case, as is it seems to work.

I guess I didn't test on Android/iOS so if it fails there then we'll need to just remove the line.

@greggman greggman enabled auto-merge (rebase) December 12, 2024 07:51
@greggman greggman merged commit 66f360d into webgpu:main Dec 12, 2024
1 check passed
@jomiq
Copy link
Contributor Author

jomiq commented Dec 12, 2024

I think it's fine. I tested locally on Chrome/Firefox/Safari with it set to <script> and it seemed to not have an issues

...

I guess I didn't test on Android/iOS so if it fails there then we'll need to just remove the line.

Just some additional info: I just saw the typo and noticed that Chrome on windows still loaded the script by doing some (in my opinion) frivolous DOM stretching... I assumed the change would be safe and "tested" by checking that it still works as expected on desktop.

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.

3 participants