-
Notifications
You must be signed in to change notification settings - Fork 39
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: slot component for dynamic plugins #184
Conversation
Thanks for the pull request, @johnvente! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
<PluggableComponent | ||
id="checkbox-form" | ||
as="communications-app-check-box-form" | ||
label="checkbox label - @openedx-plugins/communications-app-check-box-form" |
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.
This will be for this plugin @openedx-plugins/communications-app-check-box-form
|
||
<PluggableComponent | ||
id="input-form" | ||
as="communications-app-input-form" |
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.
This will be for this plugin @openedx-plugins/communications-app-input-form
<h1>Input -default</h1> | ||
</PluggableComponent> | ||
{/* this will return default child if the plugin has not been installed */} | ||
<PluggableComponent id="input-form" as="communications-app-card"> |
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.
This points to this plugin: @openedx-plugins/communications-app-card but in this case that plugin doesn't exits so this will render the card above
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #184 +/- ##
==========================================
- Coverage 83.26% 75.25% -8.02%
==========================================
Files 47 53 +6
Lines 693 699 +6
Branches 136 114 -22
==========================================
- Hits 577 526 -51
- Misses 116 171 +55
- Partials 0 2 +2 ☔ View full report in Codecov by Sentry. |
7d84303
to
d6b09c5
Compare
|
||
const component = children ? ( | ||
<PluginComponent key={id} {...pluggableComponentProps}> | ||
{children} |
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 like that the children are passed to the plugin, so it can optionally wrap the default contents 👍🏻
id="checkbox-form" | ||
as="communications-app-check-box-form" | ||
label="checkbox label - @openedx-plugins/communications-app-check-box-form" | ||
isChecked |
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.
It seems a bit strange to me that these plugins are so specific. In this case a checkbox and an input box. What if I want to replace the check box with two checkboxes, or with an input box? It seems like that sort of flexibility is missing here. With props like isChecked
being passed to the plugin, it seems like the checkbox plugin must render a checkbox. So isn't the customization too limited?
Also, I guess you're still working on this but it seems like handleCheck
is missing.
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 agree. If you have any number of children, the props will be limited only to the plugin. However, we would need to pass props for each child in a prop called 'childrenProps' or something similar, which would be an object to specify its props for each one. But I don't like that the component's props will be too many if you have many components as children. I think each plugin should be responsible for a specific task. I was wondering if there is another way to handle that customization case
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 think in general the plugins should get a very simple prop that applies no matter how many widgets are in the slot, or they should get their data from React Context, not props.
Here is one idea for how you could implement something like that:
function BulkEmailForm(props) {
// Store the state of all the form fields. The fields are controlled by plugins.
// Each key in formFieldsState is a plugin ID, and each value is {value: any, isReady: boolean}
const [formFieldsState, updateFormFieldsState] = React.useState({});
const isReady = Object.keys(formFieldsState).length > 1 && Object.values(formFieldsState).every(field => field.isReady);
const pluginProps = {formFieldsState, updateFormFieldsState};
return <Form>
<PluggableComponents id="bulk-email-form" {...pluginProps}>
<PluggableComponent id="bulk-email-form-email-recipient" {...pluginProps}>
<PluggableComponent id="bulk-email-form-email-subject" {...pluginProps}>
<PluggableComponent id="bulk-email-form-email-body" {...pluginProps}>
</PluggableComponents>
</Form>;
}
Then each plugin can manage its own state (checked or not) and data like this:
const pluginId = "new-checkbox-field";
const state = props.formFieldsState[pluginid] ?? {};
const updateState = (changes) => {
props.updateFormFieldsState(oldState => ({...oldState, [pluginId]: {...oldState[pluginId], ...changes}));
};
if (!state.isReady) {
updateState({isReady: true}); // The checkbox is not required, so allow users to submit the form whether or not this is checked.
}
return <Checkbox checked={state.value} onChange={(isChecked) => updateState({value: isChecked})} />
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.
Awesome! This is a really great idea, I will check this out. That way even each pluggableComponents can update the state of the parent and sometimes you will need to access to that data. Thanks a lot I'll be refactoring all the form that way
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.
Was this addressed, yet? Just checking.
f5e7cd6
to
2f41a23
Compare
Hi guys! @bradenmacdonald @arbrandes now |
08a3097
to
70948cd
Compare
70948cd
to
ee2dffa
Compare
a10f938
to
e3e2ce8
Compare
e3e2ce8
to
ffa1797
Compare
Hi @arbrandes sorry for the delay, I've made a refactor here with paragon dependency, I tried to remove the Thanks a lot. For now I will keep that component |
@johnvente please don't let codecov prevent you from removing |
@brian-smith-tcril Thanks! That said I've removed the |
Hi there @brian-smith-tcril! is there another change that you would like to have here or is okay? |
@johnvente I haven't had a chance to give this another full review yet. I'll let you know when I do. |
Hi guys @arbrandes @brian-smith-tcril [question] the pluggablecomponent will be something reusable, it would be better to have it in |
@johnvente, the current idea is to merge this strategy with whatever is in https://github.com/openedx/frontend-plugin-framework/, and have it live there. I imagine there are already a lot of similarities. We'll just need to make sure this use-case is also served by it. It will likely require some changes to the implementation here, but we always knew this might happen. I encourage you to start looking at what's in the FPF repo linked above and how it could be used here - or even changed to be usable here. |
@arbrandes I understand the approach of eventually merging the Widgets+UISlots and the Iframes-Plugins approaches. As I understand it however, that would mean merging this PR after the final review by @brian-smith-tcril (assuming nothing big comes up). |
Regarding the changes in this PR: as promised, we can merge this as soon as it passes review, yes. But when it comes time to generalize the code, it should probably go to improving frontend-plugin-framework (as opposed to frontend-platform). It doesn't just do iframes, by the way: it already has an implementation of UISlots itself, and there's at least one PR already open (oh wait, it just merged!) to make it even closer to what we have here. When the time comes, we'll just have to see how much needs to be changed there, how much here. (There are other things to see, as well: FPF currently requires the use of Javascript configuration files, which makes it very flexible, but tutor-mfe doesn't support it. I've just started thinking about how to do it, and would love any input.) Anyway, yeah, I think we're in alignment. Let me know if y'all wanna have a call to discuss it explicitly. |
I added some ideas on how to support JS configuration here: overhangio/tutor-mfe#199. We should take that discussion there. |
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.
Overall there's a lot of great stuff happening in here! It's wonderful to see how much of the old form has been split out into components that are easier to understand than they were when combined into one file!
I left a few comments with questions, looking forward to hearing your thoughts!
@@ -60,15 +68,20 @@ | |||
"react-router-dom": "6.15.0", | |||
"redux": "4.2.0", | |||
"regenerator-runtime": "0.13.11", | |||
"tinymce": "5.10.7" | |||
"tinymce": "5.10.7", | |||
"use-deep-compare-effect": "^1.8.1" |
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.
on the npm page for this package there is a warning that says
WARNING: Please only use this if you really can't find a way to use
React.useEffect
. There's often a better way to do what you're trying to do than a deep comparison.
It'd be great to have some documentation about what alternatives to deep compare effect you tried, and why you ended up needing to use deep compare effect.
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.
Yep, even with that warning I think that lib worth here. since the PluggableComponent
props will be an object I tried with JSON.stringify inside useEffect array dependencies but the component broke sometimes. Another approche was using lodash.isEqual
to compare props object but did not improve a lot. So at this point this lib is more solid that making something similar here.
If you think this can be changed please let me know
dispatch(formActions.updateForm({ body: value })); | ||
}; | ||
|
||
const isBodyValid = body.length > 0; |
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.
In the previous implementation, the same validation code is used to show/hide error messages and prevent POST
ing.
frontend-app-communications/src/components/bulk-email-tool/bulk-email-form/BulkEmailForm.jsx
Lines 153 to 165 in 6dd835d
const validateEmailForm = () => { | |
const subjectValid = editor.emailSubject.length !== 0; | |
const bodyValid = editor.emailBody.length !== 0; | |
const recipientsValid = editor.emailRecipients.length !== 0; | |
const scheduleValid = validateDateTime(editor.scheduleDate, editor.scheduleTime); | |
setEmailFormValidation({ | |
subject: subjectValid, | |
recipients: recipientsValid, | |
body: bodyValid, | |
schedule: scheduleValid, | |
}); | |
return subjectValid && bodyValid && recipientsValid && scheduleValid; | |
}; |
frontend-app-communications/src/components/bulk-email-tool/bulk-email-form/BulkEmailForm.jsx
Line 297 in 6dd835d
{!emailFormValidation.body && ( |
In this PR the logic has been split up and will need to be updated in multiple places if it ever changes. Have you considered ways to be able to utilize the same validation code for displaying errors and for preventing POST
?
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 was wondering about use yup which allows to have an scheme to validate the fields that we need and then show the errors in the please required but I do not implemented here since the validations are only for the length of the fields, what do you think?
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'd prefer to avoid adding another library dependency, I was thinking something more along the lines of having the existing validation logic live in a file that we can import from in multiple places.
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.
Having account that we are using multiple plugins where place do you recommend? I wouldn't like to do it in the MFE now, in another plugin?
const formData = useSelector((state) => state.form); | ||
const dispatch = useDispatch(); | ||
const { isEditMode, emailRecipients, isFormSubmitted } = formData; | ||
const hasCourseModes = courseModes.length > 1; |
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.
In the current implementation this has null checking
Line 23 in 6dd835d
const hasCourseModes = courseModes && courseModes.length > 1; |
Is there a specific reason that was removed?
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.
The reason is because courseModes
are optional as prop and by default has an empty array so is not necessary to make the null validation
courseModes: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
slug: PropTypes.string.isRequired, | ||
name: PropTypes.string.isRequired, | ||
}), | ||
), |
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.
In the current implementation the courseModes
array is required
Lines 130 to 135 in 6dd835d
courseModes: PropTypes.arrayOf( | |
PropTypes.shape({ | |
slug: PropTypes.string.isRequired, | |
name: PropTypes.string.isRequired, | |
}), | |
).isRequired, |
Is there a reason it was switched over to defaulting to an empty array instead?
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.
courseModes
sometimes was undefined even if the prop is required so I think is better to pass a default value for that prop and remove the required
const isScheduleValid = isScheduled ? scheduleDate.length > 0 && scheduleTime.length > 0 : true; | ||
const isFormValid = emailRecipients.length > 0 && subject.length > 0 | ||
&& body.length > 0 && isScheduleValid; |
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.
Same note as the one I left on BodyForm
about having validation code in multiple places. I'd prefer to not require anyone in the future that updates the validation logic to make changes in more than one place.
src/components/bulk-email-tool/bulk-email-form/BuildEmailFormExtensible/index.jsx
Outdated
Show resolved
Hide resolved
I won't be able to review this in a week, so I will check for any changes on April 1st |
Hi @brian-smith-tcril I was wondering if checking the comments about the validations of the form in one place as this comment would it be enough or do you have more concerns? I'd like to check that or/those at the same time to merge this |
Just checking in on this, @brian-smith-tcril :) |
@felipemontoya, correct me if I'm wrong, but if I remember correctly we came to the conclusion that it would be best to refactor this using frontend-plugin-framework before merging it. Can you confirm that's the case? |
After there was a clear winner in terms of the framework we will use for frontend extension we did talk and decided it was not a very good outcome to have one MFE (this) with a different architecture of extension while everyother MFE would likely end up adopting FPF. I did not close the PR because we are deploying this to some instances in the Spanish project and I don't want the branch to disappear before we actually have migrated it to FPF. To reduce noise from this PR, could you please change it to |
Thanks for the confirmation! Converted it to draft. |
Hi @felipemontoya! Checking to see if this can be closed? |
@mphilbrick211 yes. I'll close it now that we are no longer deploying to those instances. |
Description
Following this POC pluggable approach to importing plugins, we've created a reusable component that will dynamically import a module wherever it is called. The main idea is to be able to change the plugins with this mechanism, which means that instead of modifying the micro-frontend beforehand, we could change the plugin in isolation.
Using pluggable component with
BulkEmailForm
componentThe alert is a plugin as well
How does it work?
Any component can be wrapped with the pluggable component. For instance:
The prop
as
indicates where is the plugin allocated. In this case, the component will be pointing to this route:node_modules/@openedx-plugins/communications-app-card
.If the plugin was installed as a dependency, it will render the component that you have pointing to that one.
If the plugin isn't installed, it will return its children. In this case, it's this:
Multiple plugins
We've added these props to
PluggableComponent
:plugins
: This prop allows us to add multiple plugins, turningPluggableComponent
into a container where its children will be rendered. Here's an example:If the name of the plugin, for example, is @openedx-plugins/communications-app-test-component, you should provide the name without @openedx-plugins, so it would be "communications-app-test-component".
pluginsPrefix
: This prop allows us to add multiple plugins by specifying a prefix. PluggableComponent will find all plugins with that prefix and render them. For example, suppose you have three plugins:You only need to specify the prefix as a prop of PluggableComponent. In this case, "example-app-plugins" will render all plugins installed with that prefix. Here's an example:
containerPluginsProps
: This prop allows us to specify props for the container of the plugins, such as a className, styles, or other props. Here's an example:Why are we doing this?
This means that the plugin doesn't have to be installed if I'm making a feature. It's something that we can improve in the future. That way, another developer can replace that component and make more changes. This approach allows us to wrap as many components as we can, making most of the features pluggable and adaptable to various use cases. The component can be rendered without children as well, which means it won't return anything if the plugin is not installed.
How to test it
Check it out this branch and follow these steps:
Then you can run the mfe with
npm start
If you want to create another plugin you can do it in the folder plugins/communications-app/ with the structure of the other
plugins
Adding a Plugin That Lives Outside This Repository
You can do it by pointing to your npm package in your
package.json
Or installing it like this
npm install --legacy-peer-deps "@openedx-plugins/communications-app-body-email-form@npm:@openedx/plugins-communications-app-body-email-form@^1.0.0"
To create and upload a plugin to npm you could follow this guide