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

Fixes tabs dropdown button now shown onResize - #2492

Merged
jasonLaster merged 2 commits into
firefox-devtools:masterfrom
AnshulMalik:patch#2004
Apr 6, 2017
Merged

Fixes tabs dropdown button now shown onResize#2492
jasonLaster merged 2 commits into
firefox-devtools:masterfrom
AnshulMalik:patch#2004

Conversation

@AnshulMalik

Copy link
Copy Markdown
Contributor

Associated Issue: #2004

Summary of Changes

  • Added onResize with splitter

Test Plan

  • After resize, dropdown button appears

@jasonLaster jasonLaster left a comment

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.

Looking good!

Comment thread src/components/App.js Outdated
minSize: 10,
maxSize: "50%",
splitterSize: 1,
onResize: onResize.bind(this, "start"),

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.

hmm, maybe we should change this to be:

onResize: size => this.setState({startPanelSize: size})

it's a little clenaer

Comment thread src/components/App.js Outdated
this.state = {
horizontal: verticalLayoutBreakpoint.matches,
startPanelSize: 0,
endPanelSize: 0,

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.

I think @wldcordeiro might recommend that we put this state in the UI state.

What do you think?

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.

I wouldn't be against that, we already store that collapse state there.

@codecov

codecov Bot commented Mar 29, 2017

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2492   +/-   ##
=======================================
  Coverage   57.68%   57.68%           
=======================================
  Files          51       51           
  Lines        1985     1985           
  Branches      398      398           
=======================================
  Hits         1145     1145           
  Misses        840      840

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 c516a76...62abc52. Read the comment docs.

@jasonLaster jasonLaster left a comment

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.

I still saw the issue with the fix:

Comment thread src/components/App.js Outdated
startPanelCollapsed,
endPanelCollapsed,
horizontal,
endPanelSize,

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.

is endPanelSize used?

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.

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.

It's working with updated devtools-core splitter package :)

@AnshulMalik

Copy link
Copy Markdown
Contributor Author

I'll create a PR in devtools core, then it'll work fine.

@AnshulMalik

Copy link
Copy Markdown
Contributor Author

Thanks for the tweaks :)

@jasonLaster
jasonLaster merged commit 0ed3933 into firefox-devtools:master Apr 6, 2017
DanUgelow pushed a commit to DanUgelow/debugger.html that referenced this pull request May 4, 2017
* Fixes tabs dropdown button not shown onResize

* tweaks
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