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.

Sign up for GitHub

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

Jump to bottom

Replace the use of FaviconKit API #188

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Replace the use of FaviconKit API #188

merged 4 commits into from
Oct 25, 2023

Conversation

tannn-younet
Copy link
Contributor

@tannn-younet tannn-younet commented Aug 25, 2023 •
edited by jlazoff
Loading

@socket-security
Copy link

socket-security bot commented Aug 25, 2023 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
is-url 1.2.4 None +0 6.58 kB zeke
@xmldom/xmldom 0.8.10 None +0 182 kB karfau

@hesterbruikman hesterbruikman mentioned this pull request Aug 31, 2023
Merged
3 tasks
jlazoff
jlazoff previously approved these changes Sep 6, 2023
View reviewed changes
@jlazoff
Copy link

jlazoff commented Sep 6, 2023

Hi @hesterbruikman ,
Can you grant us final approval on this PR, of the person that can?

@hesterbruikman
Copy link

Hi @hesterbruikman , Can you grant us final approval on this PR, of the person that can?

@jlazoff I understand that the implementation closely resembles a draft PR on our end. As we had to pause work because of a shift in priorities: can we convert this PR to draft as well? My best guess is that this will be for 3-4 weeks

@jlazoff jlazoff added the enhancement New feature or request label Oct 12, 2023
@jlazoff
Copy link

jlazoff commented Oct 12, 2023

Hi @cuonglt-ync

Per request from @NicolasMassart

"Hi, thanks!
Do you have a staging version hosted that we could try before merging to prod?"

NicolasMassart
NicolasMassart previously approved these changes Oct 12, 2023
View reviewed changes
Copy link

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

some minor comments for consistency with the original app RP, but otherwise, good to go for deployment on a staging env for QA team to test this new page in the mobile app.

src/util/favicon.js Outdated Show resolved Hide resolved
src/util/favicon.js Outdated Show resolved Hide resolved
src/util/favicon.js Outdated Show resolved Hide resolved
src/util/favicon.js Outdated Show resolved Hide resolved
NicolasMassart
NicolasMassart approved these changes Oct 13, 2023
View reviewed changes
Copy link

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Let me know how we can test this page.
Thanks!

@jlazoff
Copy link

jlazoff commented Oct 13, 2023

Hi @cuonglt-ync ;

"Thanks a lot!
I approved the PR.
Let me know how we can test it: we just need a URL and we can set it as an env var in a QA build specific for testing it." From @NicolasMassart

@cuonglt-ync
Copy link
Collaborator

Please have the mobile team start the PR source code in development mode and connect the mobile app for testing.
If you want to test it directly on web browser, please make sure to have Allow CORS plugin installed and enabled, after that, set the value of window.__mmFavorites to the desired value and switch to Favorites tab to see the result.

@jlazoff
Copy link

jlazoff commented Oct 25, 2023

Please have the mobile team start the PR source code in development mode and connect the mobile app for testing. If you want to test it directly on web browser, please make sure to have Allow CORS plugin installed and enabled, after that, set the value of window.__mmFavorites to the desired value and switch to Favorites tab to see the result.

Looping in @hesterbruikman @NicolasMassart

@tommasini
Copy link
Contributor

This was already tested by our team, will go ahead and merge it!

@tommasini tommasini merged commit 8e63855 into master Oct 25, 2023
@tommasini tommasini deleted the dx-team/issues/1652-replace-the-use-of-faviconkit-api branch October 25, 2023 20:09
tommasini added a commit that referenced this pull request Oct 31, 2023
@tommasini
Revert "Replace the use of FaviconKit API (#188)"
a2b05fc 
This reverts commit 8e63855.
@tommasini tommasini mentioned this pull request Oct 31, 2023
Merged
tommasini added a commit that referenced this pull request Nov 2, 2023
@tommasini
Revert fav icon (#191)
4d04db8 
* Revert "fix: pooltogether dapp url link fixes #185 (#186)"

This reverts commit d2e3629.

* Revert "Replace the use of FaviconKit API (#188)"

This reverts commit 8e63855.

* remove change on all dapps TLDR from us to com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlazoff jlazoff jlazoff left review comments

@NicolasMassart NicolasMassart NicolasMassart approved these changes

Assignees

@NicolasMassart NicolasMassart

@jlazoff jlazoff

@hesterbruikman hesterbruikman

@cuonglt-ync cuonglt-ync

@tannn-younet tannn-younet

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tannn-younet @jlazoff @hesterbruikman @cuonglt-ync @tommasini @NicolasMassart