-
-
Notifications
You must be signed in to change notification settings - Fork 3
Implement Conditional New Tab in Mujo Extension #270
Implement Conditional New Tab in Mujo Extension #270
Conversation
@Temmyogunbo can you add the better title to this pr that denotes what you are fixing vs. which ticket it closes. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not going to implement Google storage for this, and you use the existing storage system.
Also with this, we will need to implement a setting for this that the user can toggle in the setting panel. Open to not making an internal plugin for this, but I think it might be good to dog food the new system. Here is an example of settings component. |
test('conditional new tab', async () => { | ||
onConditionalNewTab(NEW_TAB) | ||
|
||
expect(storage.sync.get).toBeCalled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to test that tab update is being called if the setting is true and not called if the setting is off.
Lets chat more about this tomorrow. Over-all looks great, just a few things I think we should move around. |
0f93979
to
934579c
Compare
934579c
to
f8c6a6a
Compare
@Temmyogunbo do you know if puppeteer is working at all for you? |
It seems like it is not, but also I only am seeing a small amount of the tests |
Ok so it looks like there may be some issues on how puppeteer is creating the new page, it does not seem to trigger the tabs.onCreate method. Looking into their library a little bit more to get some resolution. Also, not sure why the other test is failing it seems like that should be fine, and with the tabs.onCreate in the mock you should not need to overwrite it anymore. |
I think we might need to open an issue on Puppeteer for this, seems like it might not be triggering that event :/ |
@Temmyogunbo opened an issue for this |
#137 Description
![Screenshot 2019-10-27 at 2 27 32 AM](https://user-images.githubusercontent.com/13118321/67628187-b6914280-f861-11e9-895d-f80e19f28f30.png)
This PR implements a conditional new tab. Some tests are currently failing. Here's a snapshot below: