Frontend extensibility discussion: documenting best practices

Development Architecture
, ,
1

Over the past few releases, we’ve added quite a lot of extension points to Open edX frontend applications, allowing operators to customize the platform without forking.

These have come in the form of new frontend-plugin-framework slots, and features built using Paragon components that are themable with brand packages/design tokens.

As we continue to add extension points, reviewers are being asked to make more and more judgment calls about what should be exposed and how.

I think frontend maintainers and reviewers have a good intuitive sense of what good FPF slots and themable implementations look like, and we’ve landed wonderful extension points on the frontend.

It’d be great to have those reviewer intuitions documented!

A best practices document would be something both contributors and reviewers could reference, leading to less back-and-forth during review, and more consistency in merged PRs.

I figured instead of writing up a best practices document using my intuitions alone, it’d be good to start a thread to gather thoughts from the community.

Looking forward to hearing your thoughts!

2

Some FPF slot “rules of thumb” I’ve used when reviewing:

Do we need a new slot for this?

In my review of https://github.com/openedx/frontend-component-header/pull/644 I left the following comment:

My main question about this slot is just, “Is it needed?”

It’s not clear to me what functionality is it providing that cannot be accomplished by following the “Add Custom Components before and after Menu” example for org.openedx.frontend.layout.header_desktop_secondary_menu.v1

Is this a “unitasker” slot?

In the same review, I left another comment:

Considering the use case for this slot, I think it would make sense to instead create a slot that wraps the search button.

That would:

  • Allow plugin authors following the keepDefault pattern to either insert before, after, or both instead of only being able to insert before the search button
  • Allow plugin authors to completely replace the search button if desired
  • Effectively communicate what the slot is and where it lives.

Are the slots following the naming and life cycle ADR?

Most PRs I’ve seen adding slots do a good job of following this. I can’t think of any PRs since the ADR landed that use arbitrary slot names instead of reverse DNS.

Are aliases being added to new slots?

New slots shouldn’t include aliases.

I’ve seen this happen in a few PRs. We have quite a few slots from before the ADR that include aliases for their pre-reverse-DNS arbitrary names. It makes sense that PR authors might copy that pattern when referencing one of those when making a new slot.

3

Tokens/CSS vars

This is more of a “looking forward” section. The API contract we have for tokens is Paragon, any CSS vars that exist in the frontend outside of that aren’t part of that.

That being said, we do have a few examples of “application tokens” throughout the platform, and as of https://github.com/openedx/paragon/pull/4276 we have a way for theme authors to include those in their brand packages.

Rule of thumb: can we use a Paragon component for this?

Paragon components have theme support built-in, so by using those Instead of making a custom component in a frontend app with custom stylesheets, you get a “quick win” for themability.

CSS vars in frontend applications

There are multiple reasons someone would want to use CSS vars in a frontend application

Keeping styles DRY

Instead of repeating the same value in a lot of places in the application stylesheets, using a CSS var makes it easy to have a single source of truth. This has the side-effect of being something a brand package can override, but isn’t the goal of the CSS var.

Scoped themability

The example I like to use for this is --catalog-home-page-banner-background-color. It defaults to --pgn-color-gray-500, but changing --pgn-color-gray-500 would impact way more than just the home page banner background.

CSS vars can be changed at runtime

One form of pushback I could see for --catalog-home-page-banner-background-color is “but shouldn’t that just be handled by a slot?”

Slots offer build-time extensibility. Telling a site operator to rebuild the Catalog MFE to change the background color of the home page banner feels bad to me.

Rule of thumb (applicaton tokens): don’t add too many

In Paragon it makes sense to have pretty much everything set by tokens/css vars because it is the underlying design system. Being able to tweak all the little things and have those tweaks applied across the platform at runtime is great.

That pattern doesn’t make sense when adding vars to applications. If we had every margin-top as its own application token we’d quikcly get to a place where the entire DOM becomes an API we need to support, which just isn’t a maintainable place to be.

Rule of thumb (application tokens): meaningful tokens

If we’re adding an application specific CSS var with the intention of theme authors modifying it at runtime with brand packages, we should make sure we have a good reason to do so. Once we decide on which application tokens are “the API,” every token we add that falls into that category is something we need to maintain.

I don’t have a solid definition of “meaningful,” but things that come to mind are:

  • Is this needed?
  • Is this worth it?
  • Is it going to stick around?

If a lot of operators want this extension point, and we expect to continue to have the thing it applies to for a while (read: the foreseeable future), and handling it with build-time extension points like slots doesn’t do the trick, it feels like it might be a meaningful thing to add.

4

Thanks for starting this, @brian.smith! I can stand behind everything you proposed. I’d like to add one general “feeling” I have, and this applies to both slots and CSS variables:

As a platform, we want to have extension points at most (if not all) commonly modified locations. But these extension points are only useful if they’re stable. That means they need to have long lifecycles.

This introduces some tension: as developers, the more supported extension points we have - and the longer they live - the harder it is to make improvements to the code. We need to find a balance where it’s easy for operators to customize the platform, but at the same time not impossible for it to be improved.

The above is why the Versioning section of the slot life cycle ADR is so carefully worded. In particular:

a given slot’s default content is explicitly not part of its contract.

The point being to avoid what I call the Comprehensive Theming Trap: if everything is an API, then nothing is an API.

To follow the same principle with app-specific CSS variables will be tricky, but I believe possible. Some off-the-top-of-my-head rules of thumb:

  • Scope the variable name to the app
  • Prefer general purpose variables: think Bootstrap helper classes
  • Avoid variables that target specific DOM entities (i.e., what happens if the entity is renamed, or goes away?)

Otherwise, I think we should make clear which CSS variables are actual branding extension points we’ll support across releases, and which ones are just there for DRYness and thus passive to modification at any moment.

5

I agree, but I’m having a hard time thinking of where it would make sense to have something general purpose defined in a frontend app instead of in Paragon.

6

This would be as intended: apps should probably not have many branding extension points, precisely because it would be easy to make them too specific. (Thus making the code too difficult to modify later.)

@xitij2000, I hope you don’t mind us bringing your PR into the discussion, but I feel like the proposed CSS changes make a good point of debate:

border-color: var(--learning-sidebar-outline-heading-wrapper-border-color, var(--pgn-color-light-700));

The new CSS var (--learning-sidebar-outline-heading-wrapper-border-color) is scoped to learning (:white_check_mark:), but it’s extremely specific. If we make it a supported extension point, it means that for its lifetime we will not be able to change the layout of the outline sidebar: it will always need to have a “heading wrapper”.

Maybe that’s fine in this case, but I believe we should strive to make as few assumptions about content as possible. One simple way might be to provide a local override for the generic Paragon version:

border-color: var(--learning-sidebar-color-light-700, var(--pgn-color-light-700));

The only assumptions being made in this case are: A) there is a “Learning” app, and B) it has a sidebar.

“Ah, but then we’d have to add that new variable to any uses of --pgn-color-light-700 in the sidebar!”

Yes, exactly. Which serves as further illustration that branding extension points should not be added without careful consideration.

7

@arbrandes Glad to start the discussion based on that PR! I learned a lot in making that PR and in fact I never really used the variables sicne I got confident it wasn’t the right approach. I’ve since reverted all those changes, but haven’t updated the PR yet.

The issue I was actually facing was that I wanted to override the styles for those elements, but doing do requires that I either, a) Use a very specific selector with a !important, or b) Use a specific enough selector and then redefine something like --pgn-color-light-700 to be a completely different color. Neither was particularly appealing.

App specific variables seemed like a better solution, however once I started applying all the variables I needed I realised it’s not a viable solution, since there would be too many if I just added them naively. The PR is a good example of too many variables. So then I started exploring other ways of getting what I want.

This is unrelated to CSS variables, but a few things I do think would be useful would be:

  1. Having an app-specific CSS class at root of each app, such as a learning-app class at the root of the learning MFE would allow scoping certain selectors or variables to only apply to the learning MFE. This will in way automatially scope all variables to the learning MFE without adding any suffix.
  2. Having more css classes for key elements even if they aren’t used. If something needs its own react component chances are adding a css class for it will simplify theming.

I don’t like the idea of having --learning-sidebar-color-light-700 for another reason, which is that it’s lost a lot of semantic value, it says more about the value it has than where it’s used. I wouldn’t know this will override the border color, and if I do look it up, chances are I want to not use a light color or shade 700/ I feel that app scoped css variables should be more semantic. For instance --learning-sidebar-bg or --learning-sidebar-text, if I need something more specific like the background color of the header, then perhaps I’ll still need to use CSS, but if I generally want to have a slightly darker backgound it’ll be clearer if I set that variable. This will keep variables low but still give a good amount of extension points.

If I wanted to change the background of any “active” element then I could apply a different CSS value to --learning-sidebar-bg under the active class or :focus etc. This gives a good balance between overly-specific variable names and overly specific selectors. For instance we could have the follwing CSS to change the background color on hover:

.learning-sidebar {
  :focus {
    --learning-sidebar-bg: var(--pgn-color-primary-200);
  }
}

The css-native color-mix feature is already quite widely available, and once it reaches the threshold where it can be adopted it could allow more scoped shades, for instance if the sidebar is using different shades of light for the header, hover, etc, you could have --learning-sidebar-bgbeing lightened and darkened within the component at runtime, so a singe variable change could adapt the UI more fully.