feat!: footer legal links #403
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @asadali145! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently unmaintained. To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:
Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
asadali145
force-pushed
the
asad/footer-legal-links
branch
from
7e3966f to
eedec83
Compare
|
Hi @asadali145! When you get a moment, would you mind putting a brief description of this contribution at the top of the PR? Thank you! |
|
@mphilbrick211 Sure, I am just testing my changes with a couple of MFEs. Will add the description once I move it to the review. |
asadali145
force-pushed
the
asad/footer-legal-links
branch
from
f3518f5 to
61384a0
Compare
|
@mphilbrick211 This is ready now. |
.env.development
Outdated
| @@ -12,7 +12,6 @@ TERMS_OF_SERVICE_URL=null | |||
| PRIVACY_POLICY_URL=null | |||
| SUPPORT_EMAIL=null | |||
| STUDIO_BASE_URL=http://localhost:18010 | |||
| SHOW_ACCESSIBILITY_PAGE=false | |||
There was a problem hiding this comment.
I have tried to make it consistent with other links. The accessibility link was redirecting to <studio_base_url>/accessibility if SHOW_ACCESSIBILITY_PAGE=true. I have changed it and added a config ACCESSIBILITY_URL. Now, it will display the link if ACCESSIBILITY_URL is set and will redirect to the same URL.
There was a problem hiding this comment.
Should we also look for other usages of this variable in all other MFEs that are using it?
There was a problem hiding this comment.
MFEs will be updated once they update the header and footer versions.
There was a problem hiding this comment.
Correct. But this means that this change is a breaking one, so after squashing the commits, the one that remains should be a "feat!:".
.env.development
Outdated
| ACCESSIBILITY_URL= | ||
| ABOUT_US_URL= | ||
| HONOR_CODE_URL= | ||
| CONTACT_URL= | ||
| SUPPORT_CENTER_URL= | ||
| SUPPORT_CENTER_TEXT= | ||
| TRADEMARK_TEXT= | ||
| LOGO_ALT_TEXT= | ||
| SHOW_LOGO= |
README.rst
Outdated
| Apart from the required environment variables, this component also supports the following optional environment variable. These variables add the ability to display | ||
| custom legal links in the footer. Optional Environment Variables can also be set by the consuming micro-frontend |
There was a problem hiding this comment.
| Apart from the required environment variables, this component also supports the following optional environment variable. These variables add the ability to display | |
| custom legal links in the footer. Optional Environment Variables can also be set by the consuming micro-frontend | |
| Apart from the required environment variables, this component also supports the following optional environment variables. These variables add the ability to display | |
| custom legal links in the footer. Optional environment variables can also be set by the micro-frontend. |
.env.development
Outdated
| SITE_NAME=Open edX | ||
| SUPPORT_CENTER_TEXT= |
There was a problem hiding this comment.
Shouldn't these be set to empty strings ''? This applied to other variables as well e.g. TRADEMARK_TEXT
.env.development
Outdated
| CREDENTIALS_BASE_URL=http://localhost:18150 | ||
| CSRF_TOKEN_API_PATH=/csrf/api/v1/token | ||
| ECOMMERCE_BASE_URL=http://localhost:18130 | ||
| HONOR_CODE_URL= |
There was a problem hiding this comment.
Should we use a standard name for this? e.g. TOS_AND_HONOR_CODE. This is the one that we are already using in authn MFE .
There was a problem hiding this comment.
There is already a URL for the TOS page, this is meant to be a separate URL.
.env.development
Outdated
| @@ -12,7 +12,6 @@ TERMS_OF_SERVICE_URL=null | |||
| PRIVACY_POLICY_URL=null | |||
| SUPPORT_EMAIL=null | |||
| STUDIO_BASE_URL=http://localhost:18010 | |||
| SHOW_ACCESSIBILITY_PAGE=false | |||
There was a problem hiding this comment.
Should we also look for other usages of this variable in all other MFEs that are using it?
.env.development
Outdated
| USER_INFO_COOKIE_NAME=edx-user-info | ||
| LOGO_ALT_TEXT="Open edX Logo" |
There was a problem hiding this comment.
Do we need to make it configurable? There exists another existing variable but that has the name HEADER_LOGO_ALT_TEXT. e..g https://github.com/search?q=org%3Aopenedx+LOGO_ALT_TEXT&type=code. Should we name it FOOTER_LOGO_ALT_TEXT?
src/components/Footer.jsx
Outdated
| return ( | ||
| <footer | ||
| role="contentinfo" | ||
| className="footer d-flex border-top py-3 px-4" | ||
| > | ||
| <div className="container-fluid d-flex"> | ||
| { process.env.SHOW_LOGO |
There was a problem hiding this comment.
I think we should use getConfig from frontend-platform. An example of that is can be seen in dashboard MFE
src/components/Footer.jsx
Outdated
| {this.renderLinkIfExists(process.env.ABOUT_US_URL, 'About Us')} | ||
| {this.renderLinkIfExists(process.env.TERMS_OF_SERVICE_URL, 'Terms of Service')} | ||
| {this.renderLinkIfExists(process.env.PRIVACY_POLICY_URL, 'Privacy Policy')} | ||
| {this.renderLinkIfExists(process.env.HONOR_CODE_URL, 'Honor Code')} | ||
| {this.renderLinkIfExists(process.env.CONTACT_URL, 'Contact Us')} | ||
| {this.renderLinkIfExists(process.env.ACCESSIBILITY_URL, 'Accessibility')} | ||
| {this.renderLinkIfExists(process.env.SUPPORT_CENTER_URL, process.env.SUPPORT_CENTER_TEXT || 'FAQ & Help')} |
There was a problem hiding this comment.
We should use formatMessage with internationalization. An example of that can be seen in dashboard MFE
| @@ -14,7 +14,7 @@ exports[`<Footer /> renders correctly renders with a language selector 1`] = ` | |||
| href="http://localhost:18000" | |||
| > | |||
| <img | |||
| alt="Powered by Open edX" | |||
.env.development
Outdated
| ACCESS_TOKEN_COOKIE_NAME=edx-jwt-cookie-header-payload | ||
| ACCESSIBILITY_URL= |
There was a problem hiding this comment.
I think the accessibility is a mandatory page for a website to make it compliant with legalities. I think we should have a default fallback value for this. e.g. how we are making sure in StudioFooter. By default, we are sending the user to STUDIO_URL/accessibility.
There was a problem hiding this comment.
I am unsure why they added studio_url/accessibility as a default because no such path exists with the default installations.
There was a problem hiding this comment.
If you want then I can add it to the required configs.
|
Could you please upload images in your PR description that aren't broken? I only see a broken image link. Thanks! |
|
@asadali145 I opened a Product Review issue to go with this, openedx/platform-roadmap#324 -- can you add it to the PR description? |
@asadali145 +1 to Sarina's request. Also, it would be helpful if you could upload a screencast of the user flow inside Studio (I'd like to perform a product review on this PR, but I'm not a developer so I don't know how to set up a dev environment to do the review). Thanks! |
asadali145
force-pushed
the
asad/footer-legal-links
branch
from
00c6446 to
4d619d1
Compare
@pdpinch I have updated the PR description with the Product Review Issue link.
@ali-hugo & @sarina I have updated the screenshots. Please check.
@ali-hugo What do you mean by the user flow inside the studio? Would you like me to add Authoring MFE screenshots? |
|
This is ready for review. |
Thank you.
I think that's what I need (sorry, I don't know where the user adds and manages the footer links). Basically, what I would like to see is the process the operator has to follow to add/edit/remove the links (unless this happens inside the code, in which case I won't need to review it). I hope that makes sense! |
Yeah, now I got your point. In this case, the links are configured through code. DevOps teams can configure these links by adding the required configurations through the code. |
|
@asadali145 Thanks for confirming that the links are added in the code. I am meeting with other members of the product team tomorrow. I would like to get their feedback on this feature before I do my review. I'll get back to you once I have consolidated their thoughts. EDIT: |
|
@itsjeyd @mphilbrick211 I have an OSPR process question for you; I was asked to perform a product review on this PR but just noticed that it is not tagged as "ready for product review". Would it be best for me to hold off until that tag has been added? I don't want to interfere with the process you've put in place. EDIT: |
|
Thanks to @pdpinch for pulling this PR into the product review process. I've started the review process here: openedx/platform-roadmap#324 (comment) - would love for other stakeholders to chime in. |
asadali145
force-pushed
the
asad/footer-legal-links
branch
2 times, most recently
from
05da7e7 to
963b141
Compare
|
@arbrandes Could you please have another look? |
src/components/Footer.jsx
Outdated
| <div className="copyright-col"> | ||
| {getConfig().TRADEMARK_TEXT | ||
| && ( | ||
| <div className="trademark-text text-gray-500 small"> |
There was a problem hiding this comment.
I still think text-gray-500 is too specific to be hard-coded. Maybe remove it and reimplement it in the CSS, so it can be overriden more explicitly by theming later?
There was a problem hiding this comment.
This was a bootstrap mixin. I have updated it now.
asadali145
force-pushed
the
asad/footer-legal-links
branch
2 times, most recently
from
b82b5e7 to
1b6de13
Compare
|
Ok, I'm going to actually try it out shortly, and if it works as expected I'll approve! |
|
@itsjeyd Thank you for the detailed explanation.
Yes, that's where my confusion comes in. I will broach this topic with the Core Product Working Group and see if we can get a procedure put in place for reviewing PR's that don't have product proposals. At some point I may pull you and Michelle into the conversation for your recommendations. Thanks for your help on this. |
|
@ali-hugo, please let me know what the proper procedure ends up being: I expect to be finding a bunch of PRs in the "needs-product-review-but-doesn't-have-a-proposal" category in the near future. |
|
@itsjeyd @ali-hugo I opened openedx/platform-roadmap#324 -- does that qualify as a product proposal? |
I'll double-check but I think it's very simple: if it needs product review, everyone should stop (code reviewing/developing) the PR, make the proposal, and go through the product review process. It's a waste of (code/pr) reviewer's time to be looking at something that doesn't have product review. |
The feature ticket LGTM, thanks for creating it. I'm not part of the product working group, though, and don't have all the necessary context to answer your question definitively. |
|
Hi @pdpinch, We are still in the process of defining the product proposal / review process, so apologies for anything that's unclear.
I'm going to have to ask @jmakowski1123 to confirm this ^^. In the meantime, here's what I do know: there are two different templates for product proposals. Please visit this link, select the template that best fits your use case, and compare the requirements listed in the template with the information included in your proposal. Your proposal seems to include many of the requirements already, but you might be able to flesh it out a bit in places (e.g. by including any UX/UI designs). Sorry I couldn't provide a definite answer, but I'm sure @jmakowski1123 will be able to. |
We're in the process of defining the procedure in this document. Please keep an eye on that to see how the decisions unfold. Feel free to add any comments there too. |
asadali145
force-pushed
the
asad/footer-legal-links
branch
from
1b6de13 to
ecc5474
Compare
Add the ability to configure footer links. BREAKING CHANGE: SHOW_ACCESSIBILITY_PAGE is replaced with the ACCESSIBILITY_URL config. --------- Co-authored-by: Wassaf-Shahzad <[email protected]> Co-authored-by: Arslan Ashraf <[email protected]>
asadali145
force-pushed
the
asad/footer-legal-links
branch
from
ecc5474 to
524b907
Compare
@arbrandes Do you have an update on testing? I have rebased my PR to resolve a few conflicts. |
There was a problem hiding this comment.
I think that at least for now we're going to go on a different architectural direction. As I've been mentioning elsewhere, the footer (and of course, the header) are perfect candidates for plugin slots: this will allow operators to customize them much more flexibly than just setting a few hard-coded links.
This effort is already in progress via openedx/wg-frontend#178, and we're hoping to make it available in time for the Redwood release.
For the record, there is a competing implementation of the footer as a slot that also allows individual links to be configured via a backend API call. I figure if we're going with this level of granularity at all, that PR is a better approach:
|
Here's a forum thread for us to discuss the general question of plugin slots vs configuration: https://discuss.openedx.org/t/plugin-slots-vs-configuration/13009 |
|
@asadali145 @arbrandes It sounds like this PR is no longer needed. Unless you have any objections, we'll close it next week. |
|
@arbrandes Thanks for all of the work around the Plugin Slots. We were waiting for the Redwood release to test the FooterSlot. Here are a few things that we have noticed:
CC: @pdpinch |
|
So, as per recent conversations with @brian-smith-tcril and @davidjoy, going forward we're going to be ok with a configuration-controlled header and footer (beyond just plugin slots), provided the configuration mechanism is vetted and consistently applied, which is definitely not the case with the To summarize a few key consequences of the above:
It would be a great benefit to the community if you wrote just such a proposal for the footer. The sooner it gets reviewed and approved, the higher the chance that we can convince David to include some of it as he works on frontend-base (where both Header and Footer will live, going forward). |
|
@arbrandes Thanks for the detailed reply. I have written a proposal for the footer https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4512514079/Proposal+Customizable+Footer+for+LMS+CMS. Please add your thoughts on the proposal if any. |
Related Issues
Product Review Issue: openedx/platform-roadmap#324
mitodl/mitxonline#223
Description
Legacy LMS provides a way to configure the footer links but the MFE footer has no support for legal links. This PR adds support to configure custom legal links in the MFE footer.
Testing
Screenshots