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

Remove unnecessary classes from body #240633

Merged
merged 6 commits into from
Feb 17, 2025

Conversation

dvangonen
Copy link
Contributor

There is no need to add extra classes to the body, as these classes are specific to the editor area.

used by our fonts

These classes apply fonts, but they apply them already at the line above to mainContainer.
Thus, they are already specified in the editor area and should not be applied to the body element.

And the only thing the monaco editor has outside of its scope is monaco-parts-splash. And that's a loading screen with no text on it at all.

image

@bpasero
Copy link
Member

bpasero commented Feb 14, 2025

I am not convinced and know what kind of regressions this could cause.

@dvangonen
Copy link
Contributor Author

dvangonen commented Feb 15, 2025

I guess it shouldn't cost a lot of regressions, because those classes are just redundant..
You already set up exactly the same classes (which only set the fonts) for the actual editor container under the <body/>.

I think you have something like screenshot testing btw.
Because you actually applied a much bigger changes to the repo, for example in this pull request.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

This change works based on the assumption that nothing is being added to the body element outside the monaco-workbench element, which I think is fair, anything else would probably be something we have to address (except for the splash screen).

However, in that case I would suggest to actually go back to how it was in 1b908ce and prefix more elements with .monaco-workbench to reflect this change.

The one thing I am uncertain about how to rescue though is this one:

body.web {
position: fixed; /* prevent bounce effect */
}

@dvangonen
Copy link
Contributor Author

dvangonen commented Feb 17, 2025

Adding position: fixed to the <body> element is not a good idea..
It's create its own layer for the body element and removes it from the document's flow.

As far as I understand this is just a workaround to fix the "scroll bounce".
And it will be better to use overscroll-bahavior: none; in this case and apply it to the .monaco-workbench.

I guess overscroll-bahavior: none; should be applied there, behind the touch-action.

@Decruiseweb3

This comment was marked as spam.

@dvangonen
Copy link
Contributor Author

@Daniil8k please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@vs-code-engineering vs-code-engineering bot added this to the February 2025 milestone Feb 17, 2025
@bpasero bpasero enabled auto-merge (squash) February 17, 2025 19:29
@bpasero bpasero merged commit b4b746d into microsoft:main Feb 17, 2025
7 checks passed
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.

5 participants