fix: render favicon and siteName in title

Danyal-Faheem commented 4 months ago

Description

Fixes #667.

Changes

Added the react-helmet package to add a favicon and sitename from configuration.
Added a new Head component similar to the other MFEs.
Removed the favicon static import in favor of getting it from the configuration.
Added a discussions.page.title translation for every language (might be wrong), so that the title can be translated into other languages as well.
I have just added the discussions.app.title as the discussions.page.title in each translation for now, but I realize this could be entirely wrong. I just added this as a reference for how the change would work.

How Has This Been Tested?

Using the docs provided by Tutor, I mounted my branch with the changes and tested using the steps:

Create a superuser using the command: tutor local do createuser --staff --superuser yourusername user@email.com
Sign in to tutor with the created user
Import the demo course using the command: tutor local do importdemocourse
Visit the discussions page for the demo course at this url
The favicon and the sitename will render.

Screenshots

Adding sample screenshots of the tab view in Google Chrome for sitename: My Open edx (Default on Tutor) and the tutor-indigo favicon

Before:

After:

Merge Checklist

[ ] If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable. Not applicable
[x] Is there adequate test coverage for your changes? I have added a similar test as there was for the other MFEs.

Post-merge Checklist

[ ] Deploy the changes to prod after verifying on stage or ask @openedx/edx-infinity to do it.
[ ] 🎉 🙌 Celebrate! Thanks for your contribution.

awais-ansari commented 4 months ago

@Danyal-Faheem Thanks for your contributions! Do you need any review on this PR? Please attach screenshots of the before and the after effects of this change.

Danyal-Faheem commented 4 months ago

@Danyal-Faheem Thanks for your contributions! Do you need any review on this PR? Please attach screenshots of the before and the after effects of this change.

Hi @awais-ansari, I've added the sample before and after screenshots in the PR description.

As for the review, I definitely require one for the PR, especially for the translations as I've just added them as placeholders. I can remove them if required. I didn't add a reviewer as I didn't know who to add.

Let me know if you require something else from my side.

awais-ansari commented 3 months ago

@Danyal-Faheem Please update this PR for review.

Danyal-Faheem commented 3 months ago

Hi @awais-ansari. I've updated the PR with the requested changes. Can you let me know if anything else is required?

awais-ansari commented 3 months ago

hello @Danyal-Faheem, Lint checks are failing on this PR. Can you please have a look?

Danyal-Faheem commented 3 months ago

Hi @awais-ansari , sorry for that. The lint checks should be passing now. Can you review it for me now?

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.84%. Comparing base (12fbe7e) to head (0096cc6).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #668 +/- ## ======================================= Coverage 92.83% 92.84% ======================================= Files 158 160 +2 Lines 3307 3311 +4 Branches 899 899 ======================================= + Hits 3070 3074 +4 Misses 218 218 Partials 19 19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

awais-ansari commented 3 months ago

Other than this change all looks good to me.

Danyal-Faheem commented 3 months ago

Hi @awais-ansari , sorry for that. Can you take another look at this now?

openedx / frontend-app-discussions