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: add frontend-plugin-framework LogoSlot #528

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

brian-smith-tcril
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril commented Sep 4, 2024

Testing

I have created a testing PR to learner dash here to get a sandbox deployed. If you visit https://app.pr-449-550b00.sandboxes.opencraft.hosting/learner-dashboard/ you should see the logo links to https://openedx.org/.

Changelog

  • Removed the Logo component, renamed LinkedLogo to Logo
    • I could not find any code paths in the org where the non-linked Logo component would be rendered.
  • Removed the redundant LinkedLogo component from LearningHeader.jsx
  • Added className="logo" directly to the Logo component
    • Every use of the Logo and LinkedLogo component included this.
  • Created a new LogoSlot component that wraps the Logo in a frontend-plugin-framework PluginSlot
    • Added a README with example env.config.jsx configs for replacing and modifying the logo
    • Replaced all uses of Logo with LogoSlot

@brian-smith-tcril brian-smith-tcril force-pushed the fpf-logo-slot branch 2 times, most recently from bee173a to 318aa90 Compare September 4, 2024 20:40
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.47%. Comparing base (8a7d6ee) to head (a73ca55).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
- Coverage   70.60%   70.47%   -0.14%     
==========================================
  Files          24       25       +1     
  Lines         364      359       -5     
  Branches       96       94       -2     
==========================================
- Hits          257      253       -4     
+ Misses        105      104       -1     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pluginSlots: {
logo_slot: {
keepDefault: false,
plugins: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as an aside while we're all talking about the syntax for plugins, I have to say... that's a whole lot of code to replace a logo. 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think people generally agree that FPF config feels a bit too verbose (hence the thread https://discuss.openedx.org/t/proposal-simplified-frontend-plugin-api-config/13312)

That being said, just changing the logo image itself is still very much possible via existing config methods - wrapping the Logo component in a PluginSlot doesn't change that

Copy link
Contributor

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

Can't find anything to object to! Looks great!

@brian-smith-tcril
Copy link
Contributor Author

@sarina do you have any ideas for what a good screenshot to showcase this plugin slot might be? The ability to choose which image to use for the logo has been around for a long time, so just showing a different logo doesn't feel like the best showcase.

I'm hoping to have an image that demonstrates that people can put whatever they want in there now. One idea I had was an svg logo that lets users set the color. Another idea would be a plugin that changes the logo image based on the date for holidays or seasons. If either of those sound good to you I'll gladly put something together and get some screenshots made. I'm also super open to other ideas!

@brian-smith-tcril brian-smith-tcril merged commit 8213ee7 into openedx:master Sep 26, 2024
7 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.

3 participants