-
Notifications
You must be signed in to change notification settings - Fork 104
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 #1017
Conversation
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.
Just putting this request since I know this PR will eventually switch to using the frontend-slot-footer
library and I don't want this to accidentally get merged.
16d2d0c
to
f47b0a5
Compare
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.
Good to merge once comment is addressed.
@@ -38,6 +37,7 @@ | |||
"@fortawesome/free-regular-svg-icons": "6.5.2", | |||
"@fortawesome/free-solid-svg-icons": "6.5.2", | |||
"@fortawesome/react-fontawesome": "0.2.0", | |||
"@openedx/frontend-slot-footer": "^1.0.2", |
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 we add an explicit FPF dependency here, as in the other MFEs?
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's a straight dependency in frontend-slot-footer
https://github.com/openedx/frontend-slot-footer/blob/53987a478f615ec775137cc1bdfba982b8d0b3b6/package.json#L35 so I'm fine either way. If you'd prefer we have FPF explicitly set as a dependency in all the MFEs using frontend-slot-footer
I'm more than happy to make that change.
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.
Yeah, I guess adding a dependency if it's not used directly in the code is probably more confusing than otherwise. Let's leave it out.
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.
agreed on leaving it out until we specifically need it for Profile and it reduces the number of dependencies to update for profile
* chore(deps): update dependency glob to v10.3.15 * fix(deps): update dependency @edx/frontend-component-header to v5.3.1 * fix(deps): update dependency @edx/frontend-component-footer to v13.2.0 * fix: update footer to resolve (not so) optional peer dependency issue (openedx#1022) * feat: use frontend-plugin-framework to provide a FooterSlot (openedx#1017) * perf: add css-variables support to redwood (#6) * refactor: update package-lock * refactor: solve issues with package-lock --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Brian Smith <112954497+brian-smith-tcril@users.noreply.github.com>
No description provided.