feat: support merging base component props, custom props, and `pluginProps` via `slotOptions.mergeProps` #84

adamstankiewicz · 2024-08-23T19:06:03Z

CHANGELOG

This PR introduces several key improvements, bug fixes, and new features to the frontend-plugin-framework. The following changes have been made:

Slot Options for `PluginSlot`

Consumers can now pass the slotOptions prop to the PluginSlot component.
The current slotOptions allow opting into merging props, rather than exposing prop modifications solely within content to the widget.
This prop merging functionality supports both custom widgets and the default_contents widget, and handles special cases such as className, style, and event handlers (i.e., functions).

Validation of `PluginSlot` Children

Prop merging for the default_contents widget now only works when PluginSlot contains a single React element as its children, but you may still pass all types of children, including plain text (strings), multiple elements, etc.

Parity in Widget Prop Handling

Ensured that pluginProps are passed to the default_contents widget, achieving consistency across all widgets.
This removes the need for PluginSlot to pass some widget props both in pluginProps and directly to the default children element, as is done in a few existing usages.

React Key Warning Fix

Resolved an issue where React's key warning was triggered when a widget was wrapped via a plugin.

Default Value Adjustment for `className`

The default value for the className prop has been changed to undefined. This ensures that an empty class attribute isn't rendered in the DOM.

Automated TOC and PluginSlot Examples in Example App

The ExamplePage in the example app now automates the generation of a Table of Contents (TOC) and PluginSlot examples.
Skip links have been added to improve the discoverability of specific examples.

New `PluginOperations.Modify` Example

Added a new example of PluginOperations.Modify applied to the "default_contents" widget in the example app's ExamplePage.

NPM Script Updates

Updated the start NPM script to exclude test files by removing the --copy-files flag.
Introduced a new test:watch NPM script to streamline the testing process.

Test Enhancements

Updated the test suite to include new cases covering the prop merging functionality.

Minor Syntax Cleanup

Performed minor syntax improvements for code consistency and readability.

These changes improve the overall robustness of the framework, provide better customization options for consumers, and address some existing issues with the current implementation.

Examples

Enabling merged props within `PluginSlot` via `slotOptions`

<PluginSlot
  id="slot_name"
  // Default slotOptions
  slotOptions={{
    mergeProps: true,
  }}
>
  <span>Hello, world</span>
</PluginSlot>

codecov · 2024-08-23T19:34:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.80%. Comparing base (bf8705b) to head (751831b).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
+ Coverage   96.79%   97.80%   +1.01%     
==========================================
  Files          10       10              
  Lines         187      228      +41     
  Branches       41       58      +17     
==========================================
+ Hits          181      223      +42     
+ Misses          5        4       -1     
  Partials        1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adamstankiewicz · 2024-08-23T19:36:42Z

example/env.config.jsx

@@ -18,12 +17,28 @@ const modifyWidget = (widget) => {
  return modifiedWidget;
 };

-const wrapWidget = ({ component, idx }) => (
-  <div className="bg-warning" data-testid={`wrapper${idx + 1}`} key={idx}>


[inform/context] Passing the key here does not resolve the React console warning; The key needs to be applied to the parent wrapWidget function as opposed to a node within wrapWidget, given its acting as a component (e.g., wrapWidget is the display name of the rendered component).

adamstankiewicz · 2024-08-23T19:43:17Z

example/src/ExamplePage.jsx

 import PluginSlotWithInsert from './pluginSlots/PluginSlotWithInsert';
 import PluginSlotWithModifyWrapHide from './pluginSlots/PluginSlotWithModifyWrapHide';
 import PluginSlotWithModularPlugins from './pluginSlots/PluginSlotWithModularPlugins';
 import PluginSlotWithoutDefault from './pluginSlots/PluginSlotWithoutDefault';

+const pluginExamples = [


[inform/context] Refactored ExamplePage to iterate through pluginExamples to ensure the generated Table of Content links (below) are created in the same order as the plugin examples themselves.

adamstankiewicz · 2024-08-23T19:43:57Z

example/src/ExamplePage.jsx

+              <ul>
+                {pluginExamples.map(({ id, label }) => (
+                  <li key={id}>
+                    <a href={`#${id}`}>{label}</a>
+                  </li>
+                ))}
+              </ul>


[inform/context] Includes a small list of skip links to navigate to specific example(s), without necessarily needing to scroll to find a specific example.

adamstankiewicz · 2024-08-23T19:45:12Z

package.json

@@ -16,9 +16,10 @@
    "lint": "fedx-scripts eslint --ext .js --ext .jsx .",
    "lint:fix": "fedx-scripts eslint --fix --ext .js --ext .jsx .",
    "snapshot": "fedx-scripts jest --updateSnapshot",
-    "start": "./node_modules/.bin/fedx-scripts babel src --watch --out-dir dist --source-maps --ignore **/*.test.jsx,**/*.test.js,**/setupTest.js --copy-files",
+    "start": "fedx-scripts babel src --watch --out-dir dist --source-maps --ignore **/*.test.jsx,**/*.test.js,**/setupTest.js",


./node_modules/.bin/ isn't needed to call fedx-scripts (per other NPM scripts above).

--copy-files seems to copy test files into dist, even though they are ignored through --ignore. When running tests, it runs tests against the dist and src directories, instead of just src. Without --copy-files, npm run test will only work against src as intended.

adamstankiewicz · 2024-08-23T19:47:36Z

package.json

    "start:example": "npm --prefix example start & npm --prefix example-plugin-app start",
-    "test": "fedx-scripts jest --coverage --passWithNoTests"
+    "test": "fedx-scripts jest --coverage --passWithNoTests",
+    "test:watch": "npm run test --  --watch"


[inform/context] Exposes a "test:watch" command; same behavior as npm run test, but uses a watcher to re-run tests on file saves.

brian-smith-tcril

Overall this is looking great! Awesome cleanup and the new feature works beautifully (as I've found out by using it in openedx/frontend-component-header#528)

I left a couple small comments about naming, and a couple questions about default slotOptions. From what I can tell existing PluginSlots used without setting slotOptions should still be fine and there are just a couple places where comments aren't making that clear, but if I'm mistaken about that then I'd like to discuss what can be done to make sure that is the case and this can land as a non-breaking change!

example/env.config.jsx

example/src/pluginSlots/PluginSlotWithModifyDefaultContents.jsx

example/src/pluginSlots/PluginSlotWithModifyDefaultContentsLink.jsx

brian-smith-tcril

Super exciting to see this all come together!

src/plugins/PluginSlot.jsx

MaxFrank13

Just one nit pick on a comment. Feel free to disregard if you think it's unnecessary.

Thanks for adding this feature as well as cleaning up the areas around it! 😄

davidjoy · 2024-09-05T18:55:04Z

Oof, I'm going to have to figure out how to merge this into frontend-base, where I've been Typescript-ifying the frontend-plugin-framework code. 😬

openedx-semantic-release-bot · 2024-09-06T15:40:08Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

adamstankiewicz force-pushed the ags/modify-default-contents-props branch from f8aca45 to e068ebd Compare August 23, 2024 19:32

adamstankiewicz force-pushed the ags/modify-default-contents-props branch 2 times, most recently from 716ba3e to 3702c58 Compare August 23, 2024 20:09

adamstankiewicz commented Aug 23, 2024

View reviewed changes

adamstankiewicz force-pushed the ags/modify-default-contents-props branch 3 times, most recently from 5cb8851 to e1a762b Compare August 27, 2024 21:19

adamstankiewicz changed the title ~~feat: ensure default_contents widgets support merging props with PluginOperations.Modify~~ feat: support merging props with PluginOperations.Modify Aug 27, 2024

adamstankiewicz changed the title ~~feat: support merging props with PluginOperations.Modify~~ feat: support merging props with PluginOperations.Modify Aug 27, 2024

adamstankiewicz mentioned this pull request Aug 27, 2024

feat: support ability for consumers to add custom props / HTML attributes defined via configuration to components openedx/frontend-platform#723

Closed

4 tasks

adamstankiewicz force-pushed the ags/modify-default-contents-props branch 5 times, most recently from 6b5dde7 to 208a3a4 Compare August 28, 2024 00:22

adamstankiewicz changed the title ~~feat: support merging props with PluginOperations.Modify~~ feat: support merging base component props, custom props, and pluginProps via slotOptions.mergeProps Aug 28, 2024

adamstankiewicz force-pushed the ags/modify-default-contents-props branch 3 times, most recently from 1931be1 to 1886404 Compare August 28, 2024 00:54

feat: support merging props with PluginOperations.Modify

aff65a1

adamstankiewicz force-pushed the ags/modify-default-contents-props branch from 1886404 to aff65a1 Compare August 28, 2024 12:48

adamstankiewicz marked this pull request as ready for review August 28, 2024 13:07

brian-smith-tcril mentioned this pull request Aug 30, 2024

feat: add frontend-plugin-framework LogoSlot openedx/frontend-component-header#526

Closed

fix: provide default content object for widget modification

d115d44

brian-smith-tcril mentioned this pull request Sep 4, 2024

feat: add frontend-plugin-framework LogoSlot openedx/frontend-component-header#528

Merged

brian-smith-tcril reviewed Sep 4, 2024

View reviewed changes

adamstankiewicz force-pushed the ags/modify-default-contents-props branch from e04659a to 49df398 Compare September 4, 2024 21:45

chore: pr feedback

be17231

adamstankiewicz force-pushed the ags/modify-default-contents-props branch from 49df398 to be17231 Compare September 4, 2024 21:48

adamstankiewicz added 2 commits September 4, 2024 18:03

chore: consolidate examples

0e80e82

chore: remove content prop from LinkExample

275d3a4

brian-smith-tcril approved these changes Sep 4, 2024

View reviewed changes

MaxFrank13 reviewed Sep 5, 2024

View reviewed changes

src/plugins/PluginSlot.jsx Outdated Show resolved Hide resolved

MaxFrank13 approved these changes Sep 5, 2024

View reviewed changes

chore: pr feedback, updating comment

751831b

brian-smith-tcril merged commit f34f4df into openedx:master Sep 6, 2024
7 checks passed

openedx-semantic-release-bot added the released label Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support merging base component props, custom props, and `pluginProps` via `slotOptions.mergeProps` #84

feat: support merging base component props, custom props, and `pluginProps` via `slotOptions.mergeProps` #84

adamstankiewicz commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

adamstankiewicz Aug 23, 2024

adamstankiewicz Aug 23, 2024

adamstankiewicz Aug 23, 2024

adamstankiewicz Aug 23, 2024

adamstankiewicz Aug 23, 2024

brian-smith-tcril left a comment •

edited

Loading

brian-smith-tcril left a comment

MaxFrank13 left a comment

davidjoy commented Sep 5, 2024

openedx-semantic-release-bot commented Sep 6, 2024

feat: support merging base component props, custom props, and pluginProps via slotOptions.mergeProps #84

feat: support merging base component props, custom props, and pluginProps via slotOptions.mergeProps #84

Conversation

adamstankiewicz commented Aug 23, 2024 • edited Loading

CHANGELOG

Slot Options for PluginSlot

Validation of PluginSlot Children

Parity in Widget Prop Handling

React Key Warning Fix

Default Value Adjustment for className

Automated TOC and PluginSlot Examples in Example App

New PluginOperations.Modify Example

NPM Script Updates

Test Enhancements

Minor Syntax Cleanup

Examples

Enabling merged props within PluginSlot via slotOptions

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

adamstankiewicz Aug 23, 2024

Choose a reason for hiding this comment

adamstankiewicz Aug 23, 2024

Choose a reason for hiding this comment

adamstankiewicz Aug 23, 2024

Choose a reason for hiding this comment

adamstankiewicz Aug 23, 2024

Choose a reason for hiding this comment

adamstankiewicz Aug 23, 2024

Choose a reason for hiding this comment

brian-smith-tcril left a comment • edited Loading

Choose a reason for hiding this comment

brian-smith-tcril left a comment

Choose a reason for hiding this comment

MaxFrank13 left a comment

Choose a reason for hiding this comment

davidjoy commented Sep 5, 2024

openedx-semantic-release-bot commented Sep 6, 2024

feat: support merging base component props, custom props, and `pluginProps` via `slotOptions.mergeProps` #84

feat: support merging base component props, custom props, and `pluginProps` via `slotOptions.mergeProps` #84

adamstankiewicz commented Aug 23, 2024 •

edited

Loading

Slot Options for `PluginSlot`

Validation of `PluginSlot` Children

Default Value Adjustment for `className`

New `PluginOperations.Modify` Example

Enabling merged props within `PluginSlot` via `slotOptions`

codecov bot commented Aug 23, 2024 •

edited

Loading

brian-smith-tcril left a comment •

edited

Loading