Skip to content

Fixes apply debug console font size setting to input field#91261

Merged
isidorn merged 4 commits into
microsoft:masterfrom
jeanp413:fix-90714
Apr 7, 2020
Merged

Fixes apply debug console font size setting to input field#91261
isidorn merged 4 commits into
microsoft:masterfrom
jeanp413:fix-90714

Conversation

@jeanp413

@jeanp413 jeanp413 commented Feb 24, 2020

Copy link
Copy Markdown
Contributor

This PR fixes #90714

@isidorn isidorn self-assigned this Feb 25, 2020
isidorn
isidorn reviewed Apr 6, 2020
View reviewed changes
Comment thread src/vs/workbench/contrib/debug/browser/repl.ts Outdated
this.tree.rerender();

this.replInput.updateOptions({
fontSize,

@isidorn isidorn Apr 6, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not take the fontFamily, lineHeight computed above? Why use debugConsole.

@jeanp413 jeanp413 Apr 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Those values are for the inline css

  • lineHeight computed above has type string (? `${debugConsole.lineHeight}px` : '1.4em') and the one needed for updateOptions must be number
  • fontFamily computed above resolves to 'var(--monaco-monospace-font)' when the default font is requested

@isidorn isidorn Apr 7, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Makes sense. Thank you

@isidorn

isidorn commented Apr 6, 2020

Copy link
Copy Markdown
Collaborator

@jeanp413 great PR as usual, thank you very much for creating this.
I have left a comment in the code changes. Also I tried this out and seems to work quite well, noticed one issue though: for larger font sizes the input chevron is not focused. We probably hard code the size somewhere, and the font change breaks this. Can you maybe look into improving the centering of the chevron on the left?

Screenshot 2020-04-06 at 13 03 09

@isidorn isidorn added this to the April 2020 milestone Apr 6, 2020
@isidorn isidorn added the debug Debug viewlet, configurations, breakpoints, adapter issues label Apr 6, 2020
@isidorn

isidorn commented Apr 7, 2020

Copy link
Copy Markdown
Collaborator

@jeanp413 your comment makes sense. Once we resolve the centering of the chevron (if possible) I would be open to merge in this PR.

@jeanp413

jeanp413 commented Apr 7, 2020

Copy link
Copy Markdown
Contributor Author

Pushed some changes to center de input chevron, let me know if the css is ok

@isidorn

isidorn commented Apr 7, 2020

Copy link
Copy Markdown
Collaborator

@jeanp413 Yeah, css looks great. Let's merge this.
Thanks a lot 🍻 ☀️

@isidorn isidorn merged commit 8b919e4 into microsoft:master Apr 7, 2020
@jeanp413 jeanp413 deleted the fix-90714 branch April 7, 2020 14:10
@janosh janosh mentioned this pull request Apr 8, 2020
Closed
@github-actions github-actions Bot locked and limited conversation to collaborators May 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@isidorn isidorn isidorn left review comments

+1 more reviewer

@AdrianDeAnda AdrianDeAnda AdrianDeAnda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@isidorn isidorn

Labels

debug Debug viewlet, configurations, breakpoints, adapter issues

Projects

None yet

Milestone

April 2020

Development

Successfully merging this pull request may close these issues.

Apply debug console font size setting to input field

3 participants

@jeanp413 @isidorn @AdrianDeAnda