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

[Tagging] Discovery: Move the library-authoring MFE into the course-authoring MFE #164

Closed
bradenmacdonald opened this issue Dec 19, 2023 · 8 comments

Comments

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Dec 19, 2023

We want the "Tag Drawer" component to work identically in the course authoring experience and the library authoring experience.

There are two ways to achieve this:

  1. Move the "Tag Drawer" to its own repo, so that the Course Authoring MFE and Library Authoring MFE can both include it as a component. Or,
  2. Consolidate the Library Authoring MFE into the Course Authoring MFE

The benefits of either approach would be:

  • The Tag Drawer no longer needs to load in an iframe, so loads much faster
  • The Unit/Library page can use react-query to display the number of tags, so the number of tags will be instantly and automatically updated whenever changes are made to a component's tags.

For this ticket:

  1. Try a quick prototype of "Option 2" (consolidate) to get a sense of how much work it is.
  2. Get input from @arbrandes and @KristinAoki about which approach is preferable.
  3. Create tickets for implementing that approach.
@KristinAoki
Copy link
Member

TNL would prefer option 2. It is an idea that the team been tossing around in greater frequency as both repos continue to grow and have many overlapping components.

@rpenido
Copy link

rpenido commented Jan 3, 2024

Hi!

@KristinAoki, could you please help us with your opinion here? 😃

I made a prototype of the option 2 here:

From a technical standpoint, I didn't find any major issues. We will benefit significantly from reusing components (and we can't do it gradually, removing the duplicated components).

I'm not sure how we should coordinate it with the community. Have we done something similar before?

The worst scenario would be to do the merge and end up with two concurring library-authoring features! 😅

CC @KristinAoki @bradenmacdonald

@KristinAoki
Copy link
Member

@jesperhodge and @connorhaugh Probably would have better thoughts. Jesper did the original exploration for TNL and Connor is the SME for library-authoring.

@kdmccormick
Copy link
Member

@rpenido The library-authoring MFE has not yet been supported for production deployment in any named release. As long as that remains true, I think the merge could be performed without worrying about the library-authoring community impact at all.

@connorhaugh
Copy link

@jesperhodge has done some preliminary investigation and created some tasks for this effort, He should share those with the community

@jesperhodge
Copy link
Member

jesperhodge commented Jan 11, 2024

Hi @rpenido @kdmccormick @bradenmacdonald , I did a similar thing where I created a prototype of merging library-authoring into course-authoring (openedx/frontend-app-authoring#731). I agree that there are no big technical hurdles. I did mine only a little different in some ways, but I don't really mind about folder structure for now, as long as it is logical and doesn't create a big ball of mud.

My prototype is not complete, there are some missing parts: adding the header, using the correct links, Jest setup, displaying library block content, and a small fix for the course-import reducer. Your prototype looks like it's more complete maybe?

Anyway, I created a list of preliminary tasks in order to take this from prototype to an actual merge. I can only base that on the prototype I created, I would be interested how this would change if we use your prototype @rpenido ?

  • Decide on final folder structure
  • Implement final folder structure
  • Consolidate header (missing right now)
  • Fix display of library-block contents, and anything else we discover not working
  • fix course-import reducers (I merged them into one reducer, but this was wrong, so it needs to use two different reducers)
  • Get Jest and eslint working correctly so that tests pass and github pipelines are green
  • investigate what other infrastructure adjustments are necessary (deployment pipelines still working?) and fix as necessary
  • followup: perhaps rename this repo to authoring
  • followup: change links and redirects everywhere including edx-platform

Anyway, we are very interested in doing this merge soon, if possible.

@rpenido
Copy link

rpenido commented Jan 15, 2024

Hi @jesperhodge! Thank you for your response.
I think my prototype is in the same state as yours. I did it to gather the complexity and plan some steps. I'm not sure if I will be able to continue from here.

My task list is pretty much the same as yours. I am happy that we converged here (a sign that we are on the right path)! I just included some tasks to handle the OEP-21 deprecation process, and I think the course-import reducer task (and some pipeline adjustments) could be done earlier in the library-authoring repo before the actual merge (to reduce the merge complexity a bit). I also think that consolidating the header (and other duplicated components) could be handled in a cleanup task after the merge (also, seeking to reduce complexity).

We are also interested in the merge, but, as I said, we don't have a timeline or the resources to handle it right now. I will keep following this and hope we can help get this done in the medium term.

Also, thanks, @connorhaugh, @kdmccormick, and @KristinAoki for the insights!

@bradenmacdonald
Copy link
Contributor Author

We have agreed on a plan and we have two prototypes - what remains is just for a team to take on the work. For now I'm going to consider this discovery done and close this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

6 participants