-
Notifications
You must be signed in to change notification settings - Fork 65
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
fix: render favicon and siteName in title #668
fix: render favicon and siteName in title #668
Conversation
@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. |
@Danyal-Faheem Please update this PR for review. |
Hi @awais-ansari. I've updated the PR with the requested changes. Can you let me know if anything else is required? |
hello @Danyal-Faheem, Lint checks are failing on this PR. Can you please have a look? |
dcd0b27
to
d28aad7
Compare
Hi @awais-ansari , sorry for that. The lint checks should be passing now. Can you review it for me now? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
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.
Other than this change all looks good to me. |
Hi @awais-ansari , sorry for that. Can you take another look at this now? |
Description
Fixes #667.
Changes
react-helmet
package to add a favicon and sitename from configuration.Head
component similar to the other MFEs.discussions.page.title
translation for every language (might be wrong), so that the title can be translated into other languages as well.discussions.app.title
as thediscussions.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:
tutor local do createuser --staff --superuser yourusername user@email.com
tutor local do importdemocourse
Screenshots
Adding sample screenshots of the tab view in Google Chrome for sitename:
My Open edx
(Default on Tutor) and thetutor-indigo
faviconBefore:
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
Is there adequate test coverage for your changes?
I have added a similar test as there was for the other MFEs.
Post-merge Checklist