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

[NME] fix wheel behaviour and overscroll #12537

Merged
merged 2 commits into from
May 19, 2022
Merged

Conversation

gomes042
Copy link

@gomes042 gomes042 commented May 16, 2022


Before:

ezgif-2-d49c4d843b

After:

ezgif-2-6eac051240

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@sebavan
Copy link
Member

sebavan commented May 16, 2022

ohhhhh thanks a lot for the PR looks good to me but will add ppl who know way more than myself about the nme editor :-)

@sebavan sebavan requested review from RaananW and deltakosh and removed request for RaananW May 16, 2022 20:05
@azure-pipelines
Copy link

@gomes042
Copy link
Author

gomes042 commented May 16, 2022

Apparently it's not working on the snapshot completely as it should (probably because of index.html's). I recommend run it locally and review...

Remember to clear browser's cache please... 😅

@azure-pipelines
Copy link

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12537/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

@deltakosh deltakosh requested review from carolhmj and removed request for deltakosh May 16, 2022 21:21
@gomes042 gomes042 changed the title [NDE] fix wheel behaviour and overscroll [NME] fix wheel behaviour and overscroll May 17, 2022
Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

Thanks! Certainly a needed change.

I'm not sure I'm happy about a global script tag in the html page. This can be done in one of the components as part of the react application.
TBH I'm not too sure how many of those prevent default calls are needed, but I'm sure you have tested that. I haven't tested the pr yet, this comment is only about the code location.

Copy link
Contributor

@carolhmj carolhmj left a comment

Choose a reason for hiding this comment

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

Tested and seems to be working nicely, just +1 on Raanan that it would be better to have the event listener in a component instead of in a global script. graphEditor.tsx would be a good place.

@gomes042
Copy link
Author

The idea of the script on index.html was meant to overwrite the browser's default behavior as soon as the page is loaded, before the bundle is even loaded or any components are mounted. If the user zooms in before the page loads the whole JS bundle he will get stuck, the initial idea was to avoid it

@gomes042
Copy link
Author

gomes042 commented May 17, 2022

Which one is the best choice? Move the overwride to the graphEditor.tsx by adding and removing an eventListener in the respective componentDidMount and componentWillUnmount or leave it in the index.html for a behavior applied on browser before anything else?

Screen Shot 2022-05-17 at 14 06 33

Screen Shot 2022-05-17 at 14 03 58

Screen Shot 2022-05-17 at 14 08 21

@gomes042 gomes042 requested a review from RaananW May 17, 2022 17:14
@RaananW
Copy link
Member

RaananW commented May 18, 2022

I would still prefer keeping implementation outside of index.html. There are many different reasons for that. The main one is the separation done between the static resources and the compiled resources.

@gomes042
Copy link
Author

I would still prefer keeping implementation outside of index.html. There are many different reasons for that. The main one is the separation done between the static resources and the compiled resources.

Done 👍

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@RaananW RaananW merged commit 9aa6c7c into BabylonJS:master May 19, 2022
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.

[NME] Pinch on trackpad breaks layout
4 participants