Skip to content

feat: OpenCraft Header.#6

Closed
Kelketek wants to merge 1 commit intosandbox-headerfrom
fox/sandbox-header
Closed

feat: OpenCraft Header.#6
Kelketek wants to merge 1 commit intosandbox-headerfrom
fox/sandbox-header

Conversation

@Kelketek
Copy link
Copy Markdown
Member

@Kelketek Kelketek commented Jun 16, 2025

This MR prototypes our header design for sandboxes.

JIRA ticket: https://tasks.opencraft.com/browse/STAR-4174

Figma Design: https://www.figma.com/design/c5SgVI3M8VmkeMajJu3EFK/OpenCraft-Sandbox-Designs?node-id=0-1&p=f&t=WfH1TjTB0XE4f8lw-0

Screenshots:

image

image

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: "None" if there's no rush, "ASAP" if it's critical, or provide a specific date if there is one.

Testing instructions:

  1. Use npm ci to install the requirements
  2. Add http://local.openedx.io:8080 to the CORS whitelist of your tutor installation using a plugin. You can also just whitelist everything since you're using a local dev instance:
    hooks.Filters.ENV_PATCHES.add_item(
      (
          "openedx-lms-common-settings",
          "CORS_ORIGIN_ALLOW_ALL = True",
      )
    )
    
  3. Clone the branding repo into the public directory.
  4. In the branding repo, check out the fox/sandbox branch.
  5. npm run start to run the local server
  6. Visit http://local.openedx.io:8080 and wait for it to load and see the different header variations.

Author notes and concerns:

This PR is not intended to actually be merged. It's here to show how I've prototyped the plugins locally. I've not yet dug into the best method of distributing these changes, but I do have them working locally this way, so taking the time to have them reviewed now before doing anything else.

The mobile header also doesn't quite match the design, since implementing it looked to be more complex than we had remaining time for.

Additionally, the avatar icon is upstream's icon, since I was not sure where it would be best to place/host a replacement icon file, and it was decently similar.

@Kelketek Kelketek force-pushed the fox/sandbox-header branch from 4ec69aa to c5e289f Compare June 17, 2025 21:14
@Kelketek Kelketek force-pushed the fox/sandbox-header branch from c5e289f to 3c5eb2e Compare June 17, 2025 23:11
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (sandbox-header@3be690b). Learn more about missing BASE report.

Additional details and impacted files
@@                Coverage Diff                @@
##             sandbox-header       #6   +/-   ##
=================================================
  Coverage                  ?   68.75%           
=================================================
  Files                     ?       48           
  Lines                     ?      432           
  Branches                  ?       97           
=================================================
  Hits                      ?      297           
  Misses                    ?      132           
  Partials                  ?        3           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@xitij2000 xitij2000 left a comment

Choose a reason for hiding this comment

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

I think generally in all of these slots the pluginProps should be something that the plugin can't get in other ways. It will be part of the plugin's API and can't be changed once set. So I don't think these should be added.

slotOptions={{
mergeProps: true,
}}
pluginProps={props}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pluginProps are the API for plugin slots, so they should only pass along necessary options conservatively.

@Kelketek
Copy link
Copy Markdown
Member Author

Thanks, @xitij2000 . Pending resolution of this comment, this entire PR may be unnecessary-- though I'll likely instead want to create one for the menu activator on desktop, as you mentioned.

@Kelketek
Copy link
Copy Markdown
Member Author

@xitij2000 Rethinking it, the logo's pretty simple and the data it gets can be pulled from the context hook, so I don't even need the props there. Closing this.

@Kelketek Kelketek closed this Jun 18, 2025
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