fix: Missed favicon in Safari

[Lunyachek]

Description

The favicon is not currently displaying in Safari. After our investigation, we have found a way to fix it. We used the same approach as what was done in the account mfe. We added the favicon inclusion to the index.html file, and now the favicon is being displayed in Safari.

Merge Checklist

• If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable.
• Is there adequate test coverage for your changes?

Post-merge Checklist

• Deploy the changes to prod after verifying on stage or ask @openedx/vanguards to do it.
• 🎉 🙌 Celebrate! Thanks for your contribution.

[openedx-webhooks]

Thanks for the pull request, @Lunyachek! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (66a0d5d) 87.08% compared to head (160ce33) 87.08%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1077   +/-   ##
=======================================
  Coverage   87.08%   87.08%           
=======================================
  Files         124      124           
  Lines        2276     2276           
  Branches      634      634           
=======================================
  Hits         1982     1982           
  Misses        285      285           
  Partials        9        9           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mphilbrick211]

Hi @openedx/vanguards - this is ready for review!

[syedsajjadkazmii]

The favicon is not currently displaying in Safari. After our investigation, we have found a way to fix it. We used the same approach as what was done in the account mfe. We added the favicon inclusion to the index.html file, and now the favicon is being displayed in Safari.

Hi @Lunyachek, I am curious to learn why is this not working? https://github.com/openedx/frontend-app-authn/blob/master/src/MainApp.jsx#L37

Why is the current fix working instead?

[Lunyachek]

The favicon is not currently displaying in Safari. After our investigation, we have found a way to fix it. We used the same approach as what was done in the account mfe. We added the favicon inclusion to the index.html file, and now the favicon is being displayed in Safari.

Hi @Lunyachek, I am curious to learn why is this not working? https://github.com/openedx/frontend-app-authn/blob/master/src/MainApp.jsx#L37

Why is the current fix working instead?

Hello @syedsajjadkazmii, I can't explain why. I also was wondered and started to investigate other mfe's. And found this solution in account mfe. And it's works good.

[syedsajjadkazmii]

Hi @Lunyachek,

I tried to validate the favicon changes by cloning your fork and switching to your branch. However, after testing in Safari (Version 17.1), I couldn't observe the expected icon update. I can only see the default edX icon. To ensure accuracy, I cleared the browser cache, restarted the local development server, and thoroughly inspected the code.

Could you please provide additional details or steps to follow for testing in Safari? Any insights or suggestions on how to resolve this would be greatly appreciated.

Also could you please share before and after screenshots?

[Lunyachek]

Hi @syedsajjadkazmii. If you see the default edX icon, it means that my fix is working. Prior to the fix, we didn't have any favicon at all. There was only one letter in a grey frame. In the first screenshot (in the PR description), the favicon has been changed to our RG Theme logo. But by default, we should see the default edX icon. I attached a screenshot - before (open-release/palm.master branch) and after (my branch's changes).

[syedsajjadkazmii]

Hi @Lunyachek, I was trying to reproduce it but I am able to see the default icon on your fork's master branch. (screenshot attached).

I might be doing something wrong. Please let me know the steps on how to reproduce it.

[Lunyachek]

Hi @syedsajjadkazmii. On your screenshot I see the default edX favicon. It's ok, this is what I have achieved

[syedsajjadkazmii]

Hi @syedsajjadkazmii. On your screenshot I see the default edX favicon. It's ok, this is what I have achieved

Hi @Lunyachek, my concern is that I was able to view the default icon without your change (on master branch) means the change is not needed. Is your change needed for special scenarios? if yes, what are those scenarios? and how can I reproduce it?

[Lunyachek]

@syedsajjadkazmii, I delved deeper into this issue. I have safari 17.0 and this issue persists consistently. And I asked my colleagues to check on Safari 17.1, they confirmed my result. I can record the full video with switching between the branches, etc. and upload it somewhere

[syedsajjadkazmii]

Hi @Lunyachek,

You are right. I just found out that there is something wrong with my Safari browser because the first icon that loads on my Safari does not change. I think it just gets cached.

I will approve and merge the PR. Thanks.

[openedx-webhooks]

@Lunyachek 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.