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

Fix for the last closed tab not getting persisted#2721

Merged
jasonLaster merged 10 commits into
firefox-devtools:masterfrom
bomsy:tab-issues
May 10, 2017
Merged

Fix for the last closed tab not getting persisted#2721
jasonLaster merged 10 commits into
firefox-devtools:masterfrom
bomsy:tab-issues

Conversation

@bomsy
Copy link
Copy Markdown
Contributor

@bomsy bomsy commented Apr 24, 2017

Associated Issue: #2615 and maybe #2638

Summary of Changes

  • Make sure the pendingSelectedLocation is updated

Test Plan

Tell us a little a bit about how you tested your patch.

Example test plan:

  • I had two files opened within the top tab bar.
  • I closed both of them (all file tabs closed,
  • I refreshed the page
  • The last file should be closed.

Screenshots/Videos

persist-tabs-fix

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2017

Codecov Report

Merging #2721 into master will increase coverage by 0.75%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2721      +/-   ##
==========================================
+ Coverage    57.9%   58.65%   +0.75%     
==========================================
  Files          61       63       +2     
  Lines        2264     2368     +104     
  Branches      470      487      +17     
==========================================
+ Hits         1311     1389      +78     
- Misses        953      979      +26
Impacted Files Coverage Δ
src/actions/sources.js 37.83% <ø> (ø) ⬆️
src/reducers/sources.js 77.62% <66.66%> (-0.25%) ⬇️
src/components/SecondaryPanes/Frames/index.js 54.68% <0%> (ø)
src/components/SecondaryPanes/Frames/Group.js 7.14% <0%> (ø)
src/utils/frame.js 69.66% <0%> (+35.95%) ⬆️

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 4567fde...78cb722. Read the comment docs.

@jasonLaster
Copy link
Copy Markdown
Contributor

jasonLaster commented Apr 24, 2017

@bomsy looks great. Mind encoding those steps in a test :)

  • It can be in integration/tests/tabs or a new test that is focused on this...
  • or if you can put it in a unit test that too :)

Comment thread src/reducers/sources.js Outdated

case "CLOSE_TAB":
availableTabs = removeSourceFromTabList(state.tabs, action.url);
location = { sourceId: getNewSelectedSourceId(state, availableTabs) };
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, we might want the line number, no?

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.

Actually we would have a line number because we just closed a tab.

@bomsy
Copy link
Copy Markdown
Contributor Author

bomsy commented May 7, 2017

working on tests now!

@jasonLaster
Copy link
Copy Markdown
Contributor

@bomsy I see some unit test errors and flow type errors

@bomsy
Copy link
Copy Markdown
Contributor Author

bomsy commented May 8, 2017

yeah...still wip, i just did a quick push so i could work off another laptop

Copy link
Copy Markdown
Contributor

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

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

👍

Comment thread src/test/integration/runner.js Outdated
it("tabs", async () => await tabs(ctx));
xit("tabs - add tabs", async () => await tabs.addTabs(ctx));
xit("tabs - reload with tabs", async () => await tabs.reloadWithTabs(ctx));
xit(
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.

lol

@jasonLaster jasonLaster merged commit 657d5b3 into firefox-devtools:master May 10, 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.

2 participants