Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(plugin): add file to consolidate chrome/firefox APIs #401

Merged
merged 7 commits into from
Nov 21, 2023

Conversation

abhishek-nigam
Copy link
Contributor

Describe the PR
At 2-3 places in the codebase, we're checking if the browser is chrome or firefox, and hen using the corresponding APIs chrome.* or browser.* respectively. To reduce code repetition this adds a file browser-compat.js that exposes objects corresponding to various browser APIs such that they can be used by other functions in a browser agnostic way.

Steps to test the PR
We need to test all flows in our browser extension where browser specific APIs are being called

Expected behavior
It should work without error

Additional context
These changes were done for #325

@ghost
Copy link

ghost commented Oct 10, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@dennyabrain
Copy link
Contributor

@abhishek-nigam thank you so much! we had created an issue that was small in scope for hacktoberfest but your approach is better in the long run. We might need some time to test this out thoroughly so bear with us for that but this is a step in good direction.

At 2-3 places in the codebase, we're checking if the browser
is chrome or firefox, and hen using the corresponding APIs
chrome.* or browser.* respectively. To reduce code repetition
this adds a file browser-compat.js that exposes objects corresponding
to various browser APIs such that they can be used by other functions
in a browser agnostic way.
@abhishek-nigam
Copy link
Contributor Author

abhishek-nigam commented Oct 16, 2023

Hi @dennyabrain Good morning! Please let me know if any help/change is needed from my side. Rebased the PR to resolve some merge conflicts that had come up.

@dennyabrain
Copy link
Contributor

Hi @abhishek-nigam
We were working on a release last week and didn't want to merge your PR since it would have needed more thorough testing. @aatmanvaidya and I would most likely be able to test out your PR this week and merge. Thanks for resolving the merge conflicts. we were anticipating those as well and this reduces our workload. thank you!

@abhishek-nigam
Copy link
Contributor Author

Hi @dennyabrain That's totally understandable. Welcome and thanks for letting me know!

@aatmanvaidya
Copy link
Collaborator

hi @abhishek-nigam
I am reviewing your PR, I see some changes have to be made, will it be convenient for you to meet and we can discuss, there are a few changes I feel I can communicate better over a call than text?

@abhishek-nigam
Copy link
Contributor Author

Hi @aatmanvaidya Sure, messaging you on Slack

@dennyabrain dennyabrain linked an issue Oct 23, 2023 that may be closed by this pull request
@aatmanvaidya aatmanvaidya added the level:ticket An issue that describes a ticket (initiative>feature>ticket) label Nov 17, 2023
tarunima and others added 4 commits November 20, 2023 16:40
At 2-3 places in the codebase, we're checking if the browser
is chrome or firefox, and hen using the corresponding APIs
chrome.* or browser.* respectively. To reduce code repetition
this adds a file browser-compat.js that exposes objects corresponding
to various browser APIs such that they can be used by other functions
in a browser agnostic way.
@aatmanvaidya aatmanvaidya changed the base branch from main to development November 20, 2023 12:08
@dennyabrain dennyabrain merged commit e83d90e into tattle-made:development Nov 21, 2023
2 checks passed
@aatmanvaidya
Copy link
Collaborator

Hello @abhishek-nigam , thank you so much for the work, we have merged it!

Now the task ahead is to make the code cross platform in the other sections of the code base using the browser-compact.js file that you created. I have listed it down in this issue - #487. I will be taking this issue up

duggalsu pushed a commit to duggalsu/Uli that referenced this pull request Nov 22, 2023
…-made#401)

* refactor(plugin): add file to consolidate chrome/firefox APIs

At 2-3 places in the codebase, we're checking if the browser
is chrome or firefox, and hen using the corresponding APIs
chrome.* or browser.* respectively. To reduce code repetition
this adds a file browser-compat.js that exposes objects corresponding
to various browser APIs such that they can be used by other functions
in a browser agnostic way.
---------

Co-authored-by: Aatman Vaidya <aatmanvaidya@gmail.com>
duggalsu pushed a commit that referenced this pull request Nov 29, 2023
* refactor(plugin): add file to consolidate chrome/firefox APIs

At 2-3 places in the codebase, we're checking if the browser
is chrome or firefox, and hen using the corresponding APIs
chrome.* or browser.* respectively. To reduce code repetition
this adds a file browser-compat.js that exposes objects corresponding
to various browser APIs such that they can be used by other functions
in a browser agnostic way.
---------

Co-authored-by: Aatman Vaidya <aatmanvaidya@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
level:ticket An issue that describes a ticket (initiative>feature>ticket)
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

To make sure all the sendMessage() are cross-platfroms
4 participants