Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Persist pinned tab order when a tab is moved by any means #13271

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

petemill
Copy link
Member

@petemill petemill commented Feb 23, 2018

...such as extension, keyboard shortcut or drag.

Previously only dragging a pinned tab to a new order would cause the new order of pinned tabs to be persisted.

Fix #13264

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

On #13264

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

…tension, keyboard shortcut or drag.

Previously only dragging a pinned tab to a new order would cause the new order of pinned tabs to be persisted.

Fix #13264
@petemill petemill added this to the 0.21.x (Beta Channel) milestone Feb 23, 2018
@petemill petemill self-assigned this Feb 23, 2018
@codecov-io
Copy link

codecov-io commented Feb 23, 2018

Codecov Report

Merging #13271 into master will increase coverage by 0.08%.
The diff coverage is 90.74%.

@@            Coverage Diff             @@
##           master   #13271      +/-   ##
==========================================
+ Coverage   56.06%   56.15%   +0.08%     
==========================================
  Files         282      282              
  Lines       28060    28073      +13     
  Branches     4607     4614       +7     
==========================================
+ Hits        15732    15763      +31     
+ Misses      12328    12310      -18
Flag Coverage Δ
#unittest 56.15% <90.74%> (+0.08%) ⬆️
Impacted Files Coverage Δ
js/constants/appConstants.js 100% <ø> (ø) ⬆️
js/actions/appActions.js 18.9% <ø> (+0.1%) ⬆️
app/browser/windows.js 33.27% <100%> (+0.22%) ⬆️
app/browser/tabs.js 24.13% <33.33%> (+0.23%) ⬆️
app/common/state/pinnedSitesState.js 79.61% <88.88%> (+2.33%) ⬆️
app/browser/reducers/pinnedSitesReducer.js 98.68% <96.66%> (-1.32%) ⬇️
js/stores/windowStore.js 27.95% <0%> (+0.29%) ⬆️
app/common/lib/pinnedSitesUtil.js 80.89% <0%> (+3.37%) ⬆️
... and 2 more

@bsclifton
Copy link
Member

I believe this also fixes #8543

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Tested using keyboard shortcuts (love it) and also by dragging. Works great! Nice reworking w/ the code too 😎

@bsclifton bsclifton merged commit 4ae1b6d into master Feb 23, 2018
@bsclifton bsclifton deleted the fix/13264-pinned-tab-persist-order branch February 23, 2018 17:57
bsclifton added a commit that referenced this pull request Feb 23, 2018
Persist pinned tab order when a tab is moved by any means
@bsclifton
Copy link
Member

bsclifton commented Feb 23, 2018

master 4ae1b6d
0.22.x 63647e1
0.21.x 03f7bdf

bsclifton added a commit that referenced this pull request Feb 23, 2018
Persist pinned tab order when a tab is moved by any means
@hugobuddel
Copy link
Contributor

Does this mean that the pinned tab order should now always be the same across windows? Because they aren't on my screen (Linux, 0.22.669) and I'm wondering whether I could/should file a bug report.

@hugobuddel
Copy link
Contributor

The simplest way for me to get the pinned tab order inconsistent is by pinning a new tab. The window where the tab is pinned will have the pin on the first position, the other windows somewhere at the end. Is this worthy of a bug report?

New windows seem to have the newly pinned tab somewhere at the end. (It is not exactly at the end, the last pin is a duplicate pinned tab that I can't remove, #10179).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants