-
Notifications
You must be signed in to change notification settings - Fork 81
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
Move frontend-lib-content-components code into this repo [FC-0062] #1208
Move frontend-lib-content-components code into this repo [FC-0062] #1208
Conversation
The SelectImageModal component has been refactored so that it can also be used on the video selection page; and all its child components. Now this component is called SelectionModal and is used both for the image selector and in this new video selection screen. The assets api has been used to get the videos.
…election-gallery [FAL-3375] Feat: Video selection gallery page
…-upload-feature [BB-7157] Create new editor page for video upload
… navigation added
@KristinAoki My bad; I accidentally imported a test file into the main build. It should be working now :) |
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.
I have no objections in principle. On the contrary, I think this is a great idea, and I like the commit history preservation.
Let's just make sure that this is properly communicated, giving people enough time to raise objections if there are any we haven't thought of. In particular, I'd like to hear from 2U folk on how much time (if any) they need to prepare for the switchover on master. @KristinAoki, I imagine you have some insight into that.
ba3464e
to
af2b4dd
Compare
I got the build green (the "lint" and "codecov" checks are never going to pass due to including the history here), so this PR is ready to merge from my perspective, as soon as 2U has tested it and is comfortable with merging it :) |
@bradenmacdonald there is a merge conflict that needs to be resolved |
Thanks @KristinAoki ; it's from the Node 20 upgrade. I've fixed it now. Can I go ahead with merging this PR once the tests are confirmed passing again? I'm planning to merge it as-is (not squashing), creating a new merge commit on master and adding the 589 commits from this branch, to preserve the commit history from frontend-lib-content-components. 9 of those commits are my changes from this PR and it would be nice to squash them, but at this point I can't reasonably do that without messing up the history or creating a ton of conflicts I'll need to resolve. |
@bradenmacdonald do you have permissions that allow it to be a merge commit? My UI only allows squash or rebase as merge options |
@KristinAoki I'm going to try merging it on my local |
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
@KristinAoki It worked :) |
Description
The
frontend-lib-content-components
library is only used by this MFE, and having it in a separate repository slows down development (and installation, maintenance / version bumps, etc.). This PR moves the code into this repo.Supporting information
Discussed on Slack: https://openedx.slack.com/archives/C04BM6YC7A6/p1722545083239339
Relates to openedx/frontend-lib-content-components#499
What has actually changed?
Most of the code has not changed! The changes I've made are limited to the diff you can see in the most recent few commits:
What are the benefits?
In addition to making development much simpler, this has a major impact on reducing bundle size:
Testing instructions
Check out this branch, run
npm install
, and Ensure that editing Text, Problem, and Video components continues to work as normal.Other information
I have copied the entire git history of
frontend-lib-content-components
, so that we don't lose it. That will create a second root/initial commit in this repo, but I believe that's OK.Before we merge, it may be necessary to move some localizations in transifex. I'm not sure how to do that.