Focus tab if existing for bookmarks & history #92
Conversation
Codecov Report
@@ Coverage Diff @@
## master #92 +/- ##
==========================================
+ Coverage 57.9% 58.26% +0.35%
==========================================
Files 58 58
Lines 708 714 +6
Branches 65 65
==========================================
+ Hits 410 416 +6
Misses 267 267
Partials 31 31
Continue to review full report at Codecov.
|
|
Thanks for creating a PR for this! I think the change is helpful and we can definitely update the behavior. Saka currently kind of has a global search in the Recently Viewed mode. However this leaves the bookmarks out of the results. Maybe that can be changed (with the mode name updated) so that bookmarks are also included. |
| await browser.tabs.update(existingTab.id, { active: true }); | ||
| await browser.windows.update(existingTab.windowId, { focused: true }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Small thing but I would prefer getting rid of the return and placing the await statement in an else block here.
| @@ -63,6 +63,24 @@ test('should call appropriate activation methods', async () => { | |||
| expect(browser.tabs.update.calledOnce).toEqual(true); | |||
| }); | |||
|
|
|||
| test('should focus bookmark and history tabs if already open', async () => { | |||
| @@ -63,6 +63,24 @@ test('should call appropriate activation methods', async () => { | |||
| expect(browser.tabs.update.calledOnce).toEqual(true); | |||
| }); | |||
|
|
|||
| test('should focus bookmark and history tabs if already open', async () => { | |||
| browser.flush(); | |||
| browser.tabs.query.returns(Promise.resolve([{ id: '1', windowId: '1' }])); | |||
There was a problem hiding this comment.
Minor thing but I think there is a browser.tabs.query.yields API that lets you avoid having to use the Promise API directly. Might be wrong about this but I remember seeing it before.
There was a problem hiding this comment.
Looks like stub.resolves is the correct API. Update incoming.
| test('should focus bookmark and history tabs if already open', async () => { | ||
| browser.flush(); | ||
| browser.tabs.query.returns(Promise.resolve([{ id: '1', windowId: '1' }])); | ||
| await activateSuggestion({ |
There was a problem hiding this comment.
Can the tests in the previous test ('should call appropriate activation methods') for the bookmark and history mode be deleted since we have coverage from these?
There was a problem hiding this comment.
The previous tests ensure that the "create a new tab" behaviour still occurs when no existing tabs are open. It's a little unclear that this is the case though. I didn't want to change the file too much as it's my first PR to this repo.
|
Feedback addressed. Would be really interested in the combined search mode including bookmarks. Happy to chat or help out if need be 👍 |
Type of Change
Summary Of Changes
This change makes saka focus any existing tabs that match the URL for bookmarks & history searches, falling back to creating a new tab if none are found.
Why is this change needed?
Love the extension and I have been using it for quite a while now. I primarily use it for bookmark searching which is my default mode (although I'd love to have a unified search mode!). I commonly open bookmarks that I already have open in an active tab which will then open a new tab. I find this annoying as I end up with multiple of the same page open.
I think this change is the ideal behaviour but happy to discuss.
Does this close any open issues?
No
Checklist