Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Change horizontal and vertical realignment to only occur when line is out of bounds.#5182

Merged
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
mmcote:debugAutoScrolling
Jan 29, 2018
Merged

Change horizontal and vertical realignment to only occur when line is out of bounds.#5182
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
mmcote:debugAutoScrolling

Conversation

@mmcote
Copy link
Copy Markdown
Contributor

@mmcote mmcote commented Jan 24, 2018

Associated Issue: #5138

Here's the Pull Request Doc
https://devtools-html.github.io/debugger.html/CONTRIBUTING.html#pull-requests

Summary of Changes

-Check if the line is currently visible.
-Only realign the line to be centered if the line is out of bounds.

Test Plan

N/A

Here's the Debugger's Testing doc
https://docs.google.com/document/d/1oBMRxV8A2ag2t22YsQOxTdEv0mXKzIg0tggJjRkU1S0/edit#.
Feel free to improve it!

Before:

  • As can be seen before, on every step in the debugger the editor was being realigned horizontally and vertically.
    beforestepdebug

After:

  • After the changes the editor will only be realigned when the line goes out of the bounds of the editor.
    afterstepper

Before:

  • Negative on the margin on the right side of the scroll area was increasing the clientWidth of the scrollArea.

screen shot 2018-01-23 at 6 49 05 pm

After:

  • Removed extra margin on the right side of the scroll area.

screen shot 2018-01-23 at 6 49 21 pm

Also added some necessary functions to be mocked in Editor.spec.js, that are used in the isVisible function.

Comment thread src/utils/editor/index.js
top,
scrollArea.top,
scrollArea.top + scrollArea.clientHeight - fontHeight
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how does this work?

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.

I did this just because if your scroll view area was not exactly a multiple of the font height then the last line may be cut off when you go to debug the last line, as it will still stay in view. So instead I said that if the top is below the top of the last lines fontHeight just realign and then you can see the whole line.

Comment thread src/components/Editor/Editor.css Outdated

.CodeMirror-scroll {
margin-right: 0px;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is this needed?

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.

When I was using the codeMirror, I noticed that the clientWidth that was being returned from the getScrollInfo() was not matching what was seen in the inspector. It was off by 30px. I found that by default it had a margin-right: -30px. Although once I removed this extra negative margin, it properly represented the scroll area, the bounds of the scroll view.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jasonLaster @mmcote you might find this discussion interesting: codemirror/codemirror5#2984
not the same margin, but maybe the same reason.

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.

Thank you for bringing this to my attention :) I will make some corresponding changes when I get a chance later on in the day.

@jasonLaster jasonLaster merged commit a7135c6 into firefox-devtools:master Jan 29, 2018
jasonLaster pushed a commit that referenced this pull request Jan 31, 2018
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.

3 participants