-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added consistent footer #1
Added consistent footer #1
Conversation
ed76ff2
to
76bb7c8
Compare
There is this issue when there is a line break |
3f7acda
to
2180fe4
Compare
We don't need that "Choose Language" form. Will it still appear even when edx-platform is configured for just one language? |
@briangrossman question for you -- if the logos and link targets are configurable, can we use this for xPRO (and Residential MITx) as well? |
.env.development
Outdated
@@ -14,6 +14,16 @@ SEGMENT_KEY=null | |||
SITE_NAME=Open edX | |||
USER_INFO_COOKIE_NAME=edx-user-info | |||
LOGO_URL=https://edx-cdn.org/v3/default/logo.svg | |||
LOGO_TRADEMARK_URL=https://edx-cdn.org/v3/default/logo-trademark.svg | |||
LOGO_TRADEMARK_URL=https://rc.mitxonline.mit.edu/static/images/mit-logo.jpg |
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 about committing project-specific values to this dev file.
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 about committing project-specific values to this dev file.
These values are actually picked up from the parent MFE and we can leave it blank as well but I will update 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.
Removed project related default envs.
No, it depends if you want to display it or not. I activated to test responsiveness and if link would display correctly with it |
@pdpinch I'm not as familiar with Residential, but this footer is very similar to xPRO. xPRO also has a Support Center link, which wouldn't work in MITx Online yet. If the code would skip elements that didn't have the link target set, I'd say this works for xPRO and MITx Online at a minimum. |
We should have a support center for mitxonline in a few weeks, so it would be worth adding that.
However, there is no support center for Residential, so the link should be conditional on whether the environment variable is set.
|
Added conditional support center link with configurable |
@Wassaf-Shahzad
I think that approach would work fine in this case. Particularly with the addition of another item (Support center) |
@briangrossman |
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.
@Wassaf-Shahzad related to this image that you attached. If you look at the footer level then English
and apply
drop list are not seems me aligned.
} | ||
<div class="copyright-col"> | ||
<div class="text-gray-500 small"> | ||
{process.env.TRADEMARK_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.
It is make sense, to add a additional condition here to check for the text to be available or not. I mean, if TRADEMARK_TEXT
is not set in env then it add a empty div here
src/components/Footer.jsx
Outdated
</div> | ||
<div> | ||
<ul class="footer-sub-nav"> | ||
<li><a href={process.env.ABOUT_US_URL}>About Us</a></li> |
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 guess, these links should not be rendered if they are not setup.
42e553c
to
7e17807
Compare
The language selector is not needed in this component. I enabled it in the above image to test responsiveness. |
@Wassaf-Shahzad if we are not using the
|
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.
Ok had discussion 1-1 with Wassaf about the Language Selector that is hidden by default.
👍
The language selector is enabled by passing a prop to the footer.It is disabled by default (as no prop is passed) and we can refactor it to use and environment variable but that depends if it is needed. |
Pre-Flight checklist
What are the relevant tickets?
Closes mitodl/mitxonline#223
What's this PR do?
Updates the frontend-app-footer to MITX specifications
How should this be manually tested?
This PR can verified by the following way
Screenshots (if appropriate)
Updated Image