Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Apr 3, 2025
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.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.