-
Notifications
You must be signed in to change notification settings - Fork 96
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!: footer legal links #403
base: master
Are you sure you want to change the base?
Conversation
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. |
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. |
f3518f5
to
61384a0
Compare
@mphilbrick211 This is ready now. |
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.
I'm not too familiar with how this works, but I have a couple of questions.
Also, are these settings -- both existing and newly added -- documented anywhere?
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MFEs will be updated once they update the header and footer versions.
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.
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= |
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.
Is there any order to these settings?
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.
This is just a code review, I haven't tested it out yet.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
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.
Not sure why we removed this.
.env.development
Outdated
ACCESS_TOKEN_COOKIE_NAME=edx-jwt-cookie-header-payload | ||
ACCESSIBILITY_URL= |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bootstrap mixin. I have updated it now.
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. |
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]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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