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 course authoring mfe base url to mfe config #195

Conversation

rpenido
Copy link
Contributor

@rpenido rpenido commented Mar 1, 2024

Description

This PR adds the "COURSE_AUTHORING_MFE_BASE_URL" to the mfe config pointing to the Course Authoring MFE address.

Additional Info

We are rendering a course authoring URL in an iFrame inside the library authoring as part of the Tagging Project, so we need to know the Course Authoring MFE address.

@arbrandes
Copy link
Collaborator

Shouldn't this be part of a (or the) Library Authoring plugin? There's an experimental one here.

@rpenido
Copy link
Contributor Author

rpenido commented Mar 1, 2024

Shouldn't this be part of a (or the) Library Authoring plugin? There's an experimental one here.

As this config refers to the course-authoring url, I thing it is better to set it alongside with the others course-authoring configs. Also, if another MFE also needs it, it don't need to be set again in another plugin.

What do you think?

@bradenmacdonald
Copy link

@arbrandes I initially thought the same thing, because the Library Authoring MFE is currently the only MFE that uses this setting, but I lean toward putting it here. I think it makes more sense architecturally for the Course Authoring MFE to "announce itself" and tell other MFEs where to find it than for the Library Authoring MFE plugin to include code about how to find the Course Authoring MFE and to apply a variable about the Course Authoring MFE to all other MFEs. I'm fine with either approach though!

Copy link
Collaborator

@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 like the MFE announcing itself idea. It's a better solution than having an if statement for each MFE that would require the URL. It also standardizes on variable names.

@arbrandes arbrandes merged commit 0db2173 into overhangio:master Mar 4, 2024
1 of 2 checks passed
@rpenido rpenido deleted the rpenido/add-course-authoring-url-mfe-config branch March 18, 2024 13:02
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