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: use frontend-plugin-framework to provide a FooterSlot #331

Merged
merged 1 commit into from May 23, 2024

Conversation

brian-smith-tcril
Copy link
Contributor

Note: This removes the ability to use LOGO_POWERED_BY_OPEN_EDX_URL_SVG to set the logo in the footer. This was directly using process.env and therefore did not support the more robust configuration methods provided by https://github.com/openedx/frontend-platform/blob/master/src/config.js. The footer component itself supports using LOGO_TRADEMARK_URL to set the logo (and supports the more robust configuration methods).

Copy link

@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.

Good to merge once comments are addressed.

@@ -32,7 +32,7 @@ export const App = ({ courseMetadata, isEnabled }) => (
<main data-testid="main">
<ListView />
</main>
<Footer logo={process.env.LOGO_POWERED_BY_OPEN_EDX_URL_SVG} data-testid="footer" />

Choose a reason for hiding this comment

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

As with the other MFEs, can you please remove the var from .env.development and .env.test as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"@edx/frontend-component-header": "5.0.2",
"@edx/frontend-platform": "7.1.0",
"@edx/openedx-atlas": "^0.6.0",
"@fortawesome/fontawesome-svg-core": "^1.2.36",
"@fortawesome/free-brands-svg-icons": "^5.15.4",
"@fortawesome/free-solid-svg-icons": "^5.15.4",
"@fortawesome/react-fontawesome": "^0.2.0",
"@openedx/frontend-slot-footer": "^1.0.2",

Choose a reason for hiding this comment

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

On all the other MFEs we're adding (or keeping) the explicit FPF dependency. Shouldn't we do the same here?

@arbrandes arbrandes merged commit 64b57df into openedx:master May 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants