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

Move frontend-lib-content-components code into this repo [FC-0062] #1208

Merged
merged 590 commits into from
Aug 29, 2024

Conversation

bradenmacdonald
Copy link
Contributor

@bradenmacdonald bradenmacdonald commented Aug 9, 2024

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-unsupported/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:

  • d3d5fe0 Removed unneeded files from lib-content-components
  • 28e1956 chore: update imports, fix lint issues
  • etc. - see complete list below

What are the benefits?

In addition to making development much simpler, this has a major impact on reducing bundle size:

Before: 10 MB After: 7 MB
before after

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.

KristinAoki and others added 30 commits April 14, 2023 16:28
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
@bradenmacdonald
Copy link
Contributor Author

@KristinAoki My bad; I accidentally imported a test file into the main build. It should be working now :)

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.

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.

@bradenmacdonald
Copy link
Contributor Author

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 :)

@KristinAoki
Copy link
Member

@bradenmacdonald there is a merge conflict that needs to be resolved

@bradenmacdonald
Copy link
Contributor Author

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.

@KristinAoki
Copy link
Member

@bradenmacdonald do you have permissions that allow it to be a merge commit? My UI only allows squash or rebase as merge options

@bradenmacdonald
Copy link
Contributor Author

@KristinAoki I'm going to try merging it on my local master branch then pushing that merge commit to master. Since it's a regular push and not a force-push, I think it should work. But if not, I'll need to ping someone with admin rights to do the merge for us.

@bradenmacdonald bradenmacdonald merged commit 735d978 into openedx:master Aug 29, 2024
5 of 7 checks passed
@openedx-webhooks
Copy link

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

@bradenmacdonald
Copy link
Contributor Author

@KristinAoki It worked :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.