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

feat: allow runtime configuration #603

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

dcoa
Copy link
Contributor

@dcoa dcoa commented May 25, 2022

Description

This PR allows the app to change the favicon and the site name in the title tag at runtime and serves as a discussion.

This is part of a work to make it possible to configure the frontend applications at runtime (you can referer to this FWG issue).

This PR depend on openedx/frontend-platform#335

The changes are grouped in a component Head, that uses Helmet library and integrates internationalization to change the MFE name in the title tag according to the language.

@openedx-webhooks
Copy link

openedx-webhooks commented May 25, 2022

Thanks for the pull request, @dcoa! 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.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 25, 2022
@natabene
Copy link

@dcoa Thank you for your contribution. Please let me know once it is ready for our review.

@dcoa dcoa changed the title feat: set favicon and title in runtime feat: set favicon and title at runtime May 27, 2022
@dcoa dcoa marked this pull request as ready for review May 31, 2022 16:29
@openedx-webhooks openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 31, 2022
@dcoa
Copy link
Contributor Author

dcoa commented May 31, 2022

Hi @natabene this PR is ready for review.

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #603 (cb80cfc) into master (f9d04e4) will increase coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head cb80cfc differs from pull request most recent head 5462815. Consider uploading reports for the commit 5462815 to get more accurate results

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
+ Coverage   39.58%   39.69%   +0.11%     
==========================================
  Files         106      108       +2     
  Lines        2190     2194       +4     
  Branches      586      586              
==========================================
+ Hits          867      871       +4     
  Misses       1238     1238              
  Partials       85       85              
Impacted Files Coverage Δ
src/index.jsx 0.00% <ø> (ø)
src/head/Head.jsx 100.00% <100.00%> (ø)
src/head/messages.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MaferMazu
Copy link

@arbrandes I think you might like to take a look at this.

@dcoa
Copy link
Contributor Author

dcoa commented Jun 2, 2022

@natabene could you help me re-run the test, please?

Copy link
Contributor

@davidjoy davidjoy left a comment

Choose a reason for hiding this comment

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

The i18n/messages/*.json files should not be updated manually - they are generated and will be overridden and updated by the transifex/translations jobs. The new English language messages.js file content is sufficient. 👍🏻

I think this looks good otherwise.

@dcoa dcoa force-pushed the dcoa/runtime-favicon-title branch from 5ad28de to 0befdbc Compare June 3, 2022 22:03
Copy link
Contributor

@davidjoy davidjoy left a comment

Choose a reason for hiding this comment

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

I think this looks good! Don't think we need to wait for any of the other runtime config PRs to merge this, right? It should work regardless of whether we're actually setting config at runtime. If you agree, I can get it merged once we see the tests pass.

@dcoa
Copy link
Contributor Author

dcoa commented Jun 8, 2022

Yes, PR does not affect the actual app behavior but I wonder if it's better to use this to change the dependency once frontend-platform has been updated and backport only 1 commit to the nutmeg release.

@davidjoy
Copy link
Contributor

davidjoy commented Jun 8, 2022

Seems reasonable - I don't feel strongly either way.

@robrap
Copy link
Contributor

robrap commented Jun 15, 2022

@davidjoy: Will you be merging this?

@davidjoy
Copy link
Contributor

From @dcoa's comment above, I think we're waiting to use this PR to update the frontend-platform dependency once openedx/frontend-platform#335 merges. @dcoa, let me know if that's not what you intended and I can just merge this!

@dcoa
Copy link
Contributor Author

dcoa commented Jun 15, 2022

Yes, I'm waiting for frontend-platform to update it in this PR

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

LGTM as well.

@dcoa
Copy link
Contributor Author

dcoa commented Jul 15, 2022

Hello, just an update here, to make it possible to update the package-lock.json (with the new frontend-platform version) a peer dependency error has to be resolved (this PR does that #611 )

@dcoa dcoa force-pushed the dcoa/runtime-favicon-title branch from ad2e18b to cb80cfc Compare July 28, 2022 19:29
@dcoa dcoa changed the title feat: set favicon and title at runtime feat: allow runtime configuration Jul 28, 2022
@dcoa
Copy link
Contributor Author

dcoa commented Jul 28, 2022

Hi, @davidjoy @arbrandes someone has already fixed the peer dependency error, and I was able to update the package.json and package-lock.json the PR is ready with all changes, Could you run the test and review it?
Thanks :)

@davidjoy
Copy link
Contributor

davidjoy commented Aug 9, 2022

Argh - it says the branch has conflicts that must be resolved but isn't showing which files. Apologies, that may be because of the delay in review and something else went into the master branch.

I just tested this, seems to work well. If we can get that conflict resolved I'll push the merge button.

@dcoa dcoa force-pushed the dcoa/runtime-favicon-title branch from cb80cfc to 5462815 Compare August 9, 2022 17:20
@dcoa
Copy link
Contributor Author

dcoa commented Aug 9, 2022

Hi hi 🖖, now the conflict is resolved. 😊

@davidjoy davidjoy merged commit 1252498 into openedx:master Aug 9, 2022
@openedx-webhooks
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants