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

fix: runtime load logo for footer #4

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

johanseto
Copy link
Collaborator

Description

as you can see if logo is not falsy, the runtimevalue doesnt work.

https://github.com/openedx/frontend-component-footer/blob/master/src/components/Footer.jsx#L59

but gradebook loaded with this value

https://github.com/openedx/frontend-app-gradebook/blob/open-release/palm.master/src/App.jsx#L30

Testing Instructions

Add setting for footer logo in mfe runtime.
eg

MFE_CONFIG["LOGO_TRADEMARK_URL"] = "https://stage.nelp.gov.sa/theming/asset/images/logo.png"

Before

2024-04-23_12-40

after

2024-04-23_12-43

@johanseto
Copy link
Collaborator Author

Tested in stage
image

@johanseto johanseto requested a review from andrey-canon April 23, 2024 18:08
.env.development Outdated
@@ -7,7 +7,7 @@ LOGOUT_URL='http://localhost:18000/login'
LOGO_URL=https://edx-cdn.org/v3/default/logo.svg
LOGO_TRADEMARK_URL=https://edx-cdn.org/v3/default/logo-trademark.svg
LOGO_WHITE_URL=https://edx-cdn.org/v3/default/logo-white.svg
LOGO_POWERED_BY_OPEN_EDX_URL_SVG=https://edx-cdn.org/v3/stage/open-edx-tag.svg
LOGO_POWERED_BY_OPEN_EDX_URL_SVG=''
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you deploying the development package ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, but is a way to also make it work in local dev.

@johanseto johanseto force-pushed the jlc/fix-footer-logo branch from c181e36 to c0e0036 Compare April 24, 2024 21:06
@johanseto johanseto requested a review from andrey-canon April 24, 2024 21:06
@@ -27,7 +28,7 @@ const App = () => (
/>
</Switch>
</main>
<Footer logo={process.env.LOGO_POWERED_BY_OPEN_EDX_URL_SVG} />
<Footer logo={getConfig().LOGO_TRADEMARK_URL || process.env.LOGO_POWERED_BY_OPEN_EDX_URL_SVG} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@johanseto johanseto Apr 25, 2024

Choose a reason for hiding this comment

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

Because in that line I have to modify frontend-component-footer. That package is in NPM, and yes is possible to make a fork starting for the actual version but my concern is that to keep it to upstream that component could affect all MFEs. So keeping it here I fixed it in the gradebook where I found the footer logo was not runtime. Learning and discussions seem working...

@andrey-canon
Copy link
Collaborator

@johanseto After merging please complete the track file

@johanseto johanseto force-pushed the jlc/fix-cdn branch 5 times, most recently from ae0d7aa to 3309cdd Compare April 26, 2024 16:36
@johanseto johanseto changed the base branch from jlc/fix-cdn to open-rc/palm.nelp April 26, 2024 16:41
@johanseto johanseto force-pushed the jlc/fix-footer-logo branch from c0e0036 to 55ae694 Compare April 26, 2024 16:42
@johanseto johanseto force-pushed the jlc/fix-footer-logo branch from 55ae694 to 7e4698b Compare April 26, 2024 16:43
@johanseto johanseto merged commit 6cfb197 into open-rc/palm.nelp Apr 26, 2024
3 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.

2 participants