Footer UI Plugin Approach #405

hinakhadim · 2024-02-21T05:00:34Z

No description provided.

hinakhadim · 2024-02-21T05:04:20Z

@bradenmacdonald This is the PR where I've tried to implement Plugin UISlot approach. It is different from CourseAuthoring PR as in CourseAuthoring, plugin based approach is also implemented from backend.

bradenmacdonald · 2024-03-13T21:10:28Z

@hinakhadim So sorry for the slow reply here. I have some good news: there is now a shared implementation of the UISlot approach: https://github.com/openedx/frontend-plugin-framework - it's not quite complete yet, but is there any chance you can update this PR to use it as a test case?

CC @jsnwesson who built it :)

hinakhadim · 2024-03-17T06:01:38Z

Sure, I will update this PR

hinakhadim · 2024-03-26T11:50:58Z

Hi @bradenmacdonald , I updated footer using the UI Slot approach https://github.com/openedx/frontend-plugin-framework . Currently, the footer links are rendered perfectly as in the below image.

Here are the key steps I took:

Installed the @openedx/frontend-plugin-framework
Created a plugin folder named plugin/footer-links inside the footer and installed it.
Updated Footer.jsx and wrapped elements in PluginSlot

To test the UI Slot approach thoroughly, I:

Installed this footer by incorporating it into the learner-dashboard MFE using the module.config.js file.
Installed the @openedx/frontend-plugin-framework in learner-dashboard MFE
Then, I updated env.config.js accordingly, which effectively overrode the components as anticipated.

Below is a snippet of the modifications made:

import { PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework/src/index';
import useFooterLinks from '@edx/frontend-component-footer/components/data/fetchFooterLinks';

const LinkComponent = () => {
  const { data } = useFooterLinks();
  
  return (
    <div className="check-footer">
      <p>{data.copyright}</p>
      <div className="nav">
        {
          data.moreInfoLinks?.map((link) => (
            <li className="nav-item" key={link.name}>
              <a className="nav-link" href={link.url}>{link.title}</a>
            </li>
          ))
        }
      </div>
    </div>
  );
};

const modifyfooter = (widget) => {
  const modifiedWidget = widget;
  modifiedWidget.RenderWidget = <LinkComponent />;
  return modifiedWidget;
};

const config = {
   ....
   pluginSlots: {
    'footer-links': {
      keepDefault: true,
      plugins: [
        {
          op: PLUGIN_OPERATIONS.Modify,
          widgetId: 'default_contents',
          fn: modifyfooter,
        },
      ],
    },
  },
};

I would greatly appreciate it if you could review the changes and provide any insights or improvements you may have.

Key Observations:

It's noteworthy that we have the capability to update nested UI Slots via env.config.jsx.
Leveraging the UI Slot approach for the footer presents an opportunity for seamless customization. By creating individual plugins and incorporating them into env.config.js, contributors can make modifications without the need to fork the repository, thereby streamlining the process.

jsnwesson · 2024-03-26T15:52:15Z

package.json

@@ -1,6 +1,6 @@
 {
-  "name": "@edx/frontend-component-footer",
-  "version": "1.0.0-semantically-released",
+  "name": "@edly-io/indigo-frontend-component-footer",


Is this branch name still intended to be edly-io/*?

I've reverted the branch name and made updates. Could you please review it?

An issue I want to highlight when executing npm run snapshot, where the tests fail due to the following error.

/Users/hina/mfes/frontend-component-footer/node_modules/@openedx/frontend-plugin-framework/node_modules/@edx/frontend-platform/index.js:1 ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){export { modifyObjectKeys, camelCaseObject, snakeCaseObject, convertKeyNames, getQueryParameters, ensureDefinedConfig, parseURL, getPath } from './utils'; ^^^^^^ SyntaxError: Unexpected token 'export' 7 | import { Image } from '@edx/paragon'; 8 | import { getConfig } from '@edx/frontend-platform'; > 9 | import { PluginSlot } from '@openedx/frontend-plugin-framework'; | ^

Agh, my guess is that Jest is not handling the import statement within frontend-plugin-framework's Frontend Platform dependency for some reason. I've not seen this be an issue with other React repos, and I'm not sure what the best solution is for this, but I did notice that when npm run test is ran it shows this message:

Jest encountered an unexpected token

This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript. By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules". Here's what you can do: • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/en/ecmascript-modules for how to enable it. • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config. • If you need a custom transformation specify a "transform" option in your config. • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option. You'll find more details and examples of these config options in the docs: https://jestjs.io/docs/en/configuration.html

jsnwesson · 2024-03-27T16:56:20Z

package.json

@@ -56,7 +56,9 @@
    "@fortawesome/free-brands-svg-icons": "6.4.2",
    "@fortawesome/free-regular-svg-icons": "6.4.2",
    "@fortawesome/free-solid-svg-icons": "6.4.2",
-    "@fortawesome/react-fontawesome": "0.2.0"
+    "@fortawesome/react-fontawesome": "0.2.0",
+    "@openedx-plugins/footer-links": "file:../src/plugins/footer-links",


from the changes you made already, should this still be here?

Yes, the plugin is designed to display footer links by default upon installation. Since it is not separately available on npm, that's why included it like that .If there will be a requirement to hide these links, users have the flexibility to do so using the env.config.js file. Or Is there any better way to handle this, we can go ahead with that?

With frontend-plugin-framework, the plugins don't have to be installed as separate npm packages though. Since the source files are already part of this repository, you can just specify the plugin path using the config file. At least, that's how I thought it worked.

That's right @bradenmacdonald ! I've been importing a component purely from the local path needed to fetch it from the repo, but I've also seen a configuration in Learning MFE to do some aliasing of the plugins, which I imagine would help in the case where we want to clearly mark a component's purpose as being a "plugin".

With that, I don't think this package.json approach is necessary.

Makes sense. I also got the idea of aliasing of plugins from course-authoring MFE. I've removed it. Also, I want to ask are we going to include links in footer? I've read that ADR and that's why included the footer-links.

bradenmacdonald · 2024-03-28T19:42:21Z

src/components/Footer.jsx

+                    </a>
+                  </li>
+                </PluginSlot>
+              </ul>


I think this format is a little too prescriptive. We shouldn't be including a tutor logo/slot by default, because not everyone uses Tutor to deploy it. And this kind of assumes there will be either two logos, one logo, or none. What would be much more flexible is something like this:

<PluginSlot as="footer" id="mfe-global-footer" className="footer border-top py-3 px-4" role="contentinfo"> <PluginWidget id="default-poweredby-logo"><a ...><img ... /></a></PluginWidget> <PluginWidget id="language-selector">...</PluginWidget> <PluginWidget id="copyright-text">...</PluginWidget> </PluginSlot>

And then for your Tutor plugin, in the config you have something like:

Hide the default-poweredby-logo widget.

Insert the new poweredby-tutor-openedx widget before the default-poweredby-logo widget.

But @jsnwesson is it possible to define several default widgets in one slot like this with the plugin framework?

Plugin Framework currently does not have any component named PluginWidget. In the latest commit, I've added PluginSlot instead of PluginWidget. Can you please check again?
Furthermore, on what basis, we are going to decide whether we have to use PluginSlot or PluginWidget (if it will be added in plugin framework)?
What I understand is that a list is PluginSlot for example, and its items will be Widgets. Here, in footer, all are different elements (logo, lang selector, copyright) and not list. Then, why are we treating them like widgets? Any point which I'm missing.

Hi,
Would like to hear the concerns from your side.

Hi @hinakhadim and @bradenmacdonald , huge apologies for the delay in response!
I think one of the confusions here was the naming convention differences between Braden's UI Plugin approach and Frontend Plugin Framework's ("FPF").

In FPF, there are only PluginSlots, not PluginWidgets (and there is no plan to bring that name into the library for any use case).

FPF takes Braden's UI approach of using PluginOperations to define what widgets will go into the PluginSlot as defined by the env.config.js file.

Lastly, and possibly most importantly, any content inside a PluginSlot is considered the defaultContent. That defaultContent is treated as one children body and can either be included or hidden in production by modifying the keepDefault attribute for that PluginSlot via env.config.js. (Source)

And to confirm with you @hinakhadim , I noticed that inside the "parent" PluginSlot with the id mfe-global-footer, I see that it has four PluginSlots as part of its defaultContent body.
Is the desire here so that each of those four "child" PluginSlots can be modified in the env.config.js, or was it there a misunderstanding around if they each needed to be wrapped by a PluginSlot?
For example, if you didn't want to customize each individual component in the defaultContent of the "parent" PluginSlot, you could rewrite this as:

<PluginSlot as="footer" id="mfe-global-footer" className="footer border-top py-3 px-4" role="contentinfo"> <a className="d-block" href={config.LMS_BASE_URL} aria-label={intl.formatMessage(messages['footer.logo.ariaLabel'])} > <img style={{ maxHeight: 45 }} src={logo || config.LOGO_TRADEMARK_URL} alt={intl.formatMessage(messages['footer.logo.altText'])} /> </a> <FooterLinks /> {showLanguageSelector && ( <LanguageSelector options={supportedLanguages} onSubmit={onLanguageSelected} /> )} <span className="copyright-site mt-2">{intl.formatMessage(messages['footer.copyright.text'])}</span> </PluginSlot>

@hinakhadim @jsnwesson Sorry for the confusion. I was hoping there would be some way to have multiple widgets in a plugin slot in FPF. It seems like is it possible if you define it in a config, but it's not currently possible to define multiple widgets as the default content without setting a config override.

I do feel that for a use case like this, it's nice to have several default widgets in one slot, but you could either just create them as several slots inside a bigger slot or one slot with one default content that is completely overwritten by a plugin even though that may cause a lot of duplicate code.

Is the desire here so that each of those four "child" PluginSlots can be modified in the env.config.js, or was it there a misunderstanding around if they each needed to be wrapped by a PluginSlot?

@jsnwesson My thinking was that when someone wants to change only logo, they can target the logo PluginSlot. if someone doesn't require this footer and wants to introduce their own they can target the parent PluginSlot.

As @bradenmacdonald said previously, giving option for only one bigger slot causes lot of duplications. Someone wants to change logo has to completely override the whole footer component which causes frustration. What are your thoughts, Jason?

Knowing that this PluginSlots within a PluginSlot implementation is intentional, I think it works! My team has a similar thing in mind for the Course List module in Learner Dashboard MFE.

jsnwesson · 2024-04-16T23:35:21Z

src/plugins/footer-links/package.json

@@ -0,0 +1,18 @@
+{


@bradenmacdonald if having a package.json for a /plugins directory in your Course Authoring plugins, I'm assuming this isn't needed any longer now that FPF would allow for the components to be plugged in using DirectPlugin. Just wanted to get a confirmation that this is probably a remnant of a different plugin approach.

@jsnwesson The pluggable "Pages & Resources" section of Course Authoring uses a different plugin approach which still uses package.json files for each plugin. But I don't think it's needed here, nor with FPF in general.

Yes, this is no longer needed now as I am importing FooterLink from path.

jsnwesson · 2024-04-18T20:26:45Z

In terms of implementing FPF library for this PR, I'd say it's being used appropriately!

…-framework

hinakhadim · 2024-04-24T08:27:52Z

Are we going to merge it? As now, you are implementing learner-dashboard mfe using FPF. Will this footer be compatible with other MFEs as they don't build using FPF?

jsnwesson · 2024-05-10T22:14:28Z

hi @hinakhadim , I'll continue to try to provide context with regards to using FPF, but I think @bradenmacdonald should have the last word on approving this PR.

In terms of concern for other MFEs not having FPF as a dependency, I'll note that the recently merged PR #423 adds FPF as a peer dependency, so I believe that resolves any possible issue there.

Also pinging @arbrandes in case you have any comments on this PR.

bradenmacdonald · 2024-05-13T17:30:17Z

Oh, I don't really know much context on the footer and don't have commit rights on this repo, so I wasn't planning to be the main reviewer here. I just want to see a flexible approach where the default will work for various use cases (edx.org, tutor installs, etc.) but can be customized easily.

I'm happy with what I see in the PR now, where you can either override the whole footer, or just individual parts of it 👍🏻

jsnwesson · 2024-05-13T21:39:19Z

Ah! Then in that case @hinakhadim , I'm happy to continue reviewing this PR once the conflicts are resolved (which I think is related to the PR that was merged last week that also brought in the FPF package. While you resolve the conflict, I recommend looking at those changes, which includes adding a /components/footer-slot directory that you might be able to add your components to as is appropriate.

brian-smith-tcril · 2024-05-14T16:00:37Z

@jsnwesson the idea of #423 was to add FPF as an optional peer dependency (which ended up not working as expected openedx/frontend-app-profile#1022)

The thought process was then to provide FooterSlot from a new repo openedx/wg-frontend#178 (comment) instead of directly from frontend-component-footer, but it seems there's still conversation to be had as to what the best approach is here.

arbrandes

It's unfortunate (and on me for not checking open PRs) that I wasn't aware of this before embarking on openedx/wg-frontend#178 with @brian-smith-tcril, but at this point I think we're going with the solution described there for a potential Redwood backport for the simple fact it does less, and therefore should be an easier sell as a backport.

We can still discuss the improvements shown here separately, of course. But I have concerns:

I'd like to avoid adding a tie to edx-platform (via that branding API) when the same thing could just as easily be provided by an actual frontend plugin. I talked about this before in the ADR about this that ended up closed. There are conversations there about different points of view which are worthy of discussion (such as having a potentially generalized branding API based on hooks and backend plugins, as opposed to being dedicated to footer links), but we should tackle those before merging it here.
I'm not sold on the idea of having multiple nested slots that can be individually overriden for something as simple as the footer. Remember that each slot is an explicit API extension point that we are promising to support for as long as we can going forward. The more generic and simple we can make the API, the less potential breakage users will have to deal with down the line.

(I'm requesting changes not as an objection in general, but as a signal that we should talk about it before going this direction.)

arbrandes · 2024-05-15T15:14:05Z

I've started a forum thread for us to discuss the question of plugin slots vs configuration. As noted there, I intend to eventually create an OEP (or two) based on that discussion so that we end up with an official stance on when-where-how to use config vars, and when-where-how to create plugin (slots) instead.

https://discuss.openedx.org/t/plugin-slots-vs-configuration/13009

hinakhadim mentioned this pull request Feb 21, 2024

Convert "Pages & Resources" page to a plugin system openedx/frontend-app-authoring#638

Merged

hinakhadim force-pushed the hina/footer-ui-plugin branch from c3466c4 to 1240cdc Compare March 26, 2024 11:19

jsnwesson reviewed Mar 26, 2024

View reviewed changes

jsnwesson reviewed Mar 27, 2024

View reviewed changes

bradenmacdonald reviewed Mar 28, 2024

View reviewed changes

jsnwesson reviewed Apr 16, 2024

View reviewed changes

hinakhadim added 6 commits April 22, 2024 10:11

feat: Update styling for indigo footer

43ab897

feat: implement footer using UI Plugin approach using frontend-plugin…

e483f0a

…-framework

fix: minor fixes

8d3704d

fix: remove tutor logo and plugin alias

d11cc96

fix: remove unused package.json in plugin folder

d29590a

fix: rebase with openedx/master andd update snapshot

09af1b4

hinakhadim force-pushed the hina/footer-ui-plugin branch from d6c946f to 09af1b4 Compare April 22, 2024 05:30

This was referenced May 13, 2024

Customize Footer links in MFEs openedx/platform-roadmap#324

Open

feat: implement FooterSlot openedx/frontend-slot-footer#2

Merged

arbrandes requested changes May 14, 2024

View reviewed changes

This was referenced May 15, 2024

feat!: footer legal links #403

Open

feat: ability to point header logo to other locations openedx/frontend-component-header#479

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Footer UI Plugin Approach #405

Footer UI Plugin Approach #405

hinakhadim commented Feb 21, 2024

hinakhadim commented Feb 21, 2024

bradenmacdonald commented Mar 13, 2024

hinakhadim commented Mar 17, 2024

hinakhadim commented Mar 26, 2024

jsnwesson Mar 26, 2024

hinakhadim Mar 27, 2024

jsnwesson Mar 27, 2024

jsnwesson Mar 27, 2024

hinakhadim Mar 28, 2024

bradenmacdonald Mar 28, 2024

jsnwesson Mar 29, 2024

hinakhadim Mar 31, 2024

bradenmacdonald Mar 28, 2024 •

edited

Loading

hinakhadim Mar 31, 2024

hinakhadim Apr 9, 2024

jsnwesson Apr 16, 2024

jsnwesson Apr 16, 2024

bradenmacdonald Apr 17, 2024

hinakhadim Apr 18, 2024

jsnwesson Apr 18, 2024

jsnwesson Apr 16, 2024

bradenmacdonald Apr 17, 2024

hinakhadim Apr 18, 2024

jsnwesson commented Apr 18, 2024

hinakhadim commented Apr 24, 2024

jsnwesson commented May 10, 2024

bradenmacdonald commented May 13, 2024

jsnwesson commented May 13, 2024

brian-smith-tcril commented May 14, 2024

arbrandes left a comment •

edited

Loading

arbrandes commented May 15, 2024

Footer UI Plugin Approach #405

Are you sure you want to change the base?

Footer UI Plugin Approach #405

Conversation

hinakhadim commented Feb 21, 2024

hinakhadim commented Feb 21, 2024

bradenmacdonald commented Mar 13, 2024

hinakhadim commented Mar 17, 2024

hinakhadim commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnwesson commented Apr 18, 2024

hinakhadim commented Apr 24, 2024

jsnwesson commented May 10, 2024

bradenmacdonald commented May 13, 2024

jsnwesson commented May 13, 2024

brian-smith-tcril commented May 14, 2024

arbrandes left a comment • edited Loading

Choose a reason for hiding this comment

arbrandes commented May 15, 2024

bradenmacdonald Mar 28, 2024 •

edited

Loading

arbrandes left a comment •

edited

Loading