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

Stop scrolling when we toggle tools#3225

Merged
jbhoosreddy merged 2 commits into
firefox-devtools:masterfrom
jasonLaster:fconsole
Jun 24, 2017
Merged

Stop scrolling when we toggle tools#3225
jbhoosreddy merged 2 commits into
firefox-devtools:masterfrom
jasonLaster:fconsole

Conversation

@jasonLaster
Copy link
Copy Markdown
Contributor

@jasonLaster jasonLaster commented Jun 24, 2017

Associated Issue: #3168 #3033

Summary of Changes

  • the problem was that we were unmounting codemirror when we toggled tools. We fix that, by first checking if the app is visible when we might unmount the editor.

Test Plan

  • simple integration test for toggling tools

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 24, 2017

Codecov Report

Merging #3225 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3225   +/-   ##
======================================
  Coverage    47.9%   47.9%           
======================================
  Files          98      98           
  Lines        4064    4064           
  Branches      835     835           
======================================
  Hits         1947    1947           
  Misses       2117    2117

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93750a7...a6510a9. Read the comment docs.

pressKey(dbg, "inspector");
pressKey(dbg, "debugger");

is(dbg.win.cm.getScrollInfo().top, 284);
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.

we can always test more invariants like bps, search state... but I prefer a minimal test that correlates with the behavior. We should to more tests on the unit side

Comment thread src/utils/ui.js
* because it is more reliable than either checking a focus state or
* the visibleState or hidden property.
*/
export function isVisible() {
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.

does isVisible feels vague, maybe something like isAppVisible is a bit clearer?

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.

yeah - that's fair

Copy link
Copy Markdown
Contributor

@bomsy bomsy left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@jbhoosreddy jbhoosreddy left a comment

Choose a reason for hiding this comment

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

LGTM

@jbhoosreddy jbhoosreddy merged commit 1e8d1bc into firefox-devtools:master Jun 24, 2017
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