OEP 0028: Content theming in React (Draft) #79

taranjeet · 2018-08-23T21:25:47Z

It aims to add guidelines for how to create and use React components, so that content modification becomes easy.

This OEP is based on this discussion and this gist

Note: As OpenCraft, our role is to define a proper solution and ensure that the future developments move in the right direction, but not committing on developing any of this at the moment.

openedx-webhooks · 2018-08-23T21:26:01Z

Thanks for the pull request, @taranjeet! I've created OSPR-2565 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

mduboseedx · 2018-08-24T16:56:48Z

@taranjeet Thank you for the proposal. Feel free to ping me when you feel it is ready for an edX review

oeps/oep-0028-content-theming-in-react.rst

bradenmacdonald · 2018-08-24T18:26:56Z

oeps/oep-0028-content-theming-in-react.rst

+Decision
+--------
+
+* We will build small modular components which will be non-customizable, stateless and contain HTML, and certain logic. We can see an example of a smaller component SiteLogo, which will later be used in a customizable component _Header.


It's not really explained here that there are meant to be two types of components: internal/non-customizable and customizable ones.

At first read, this sentence just sounds like "we will build everything out of small components that cannot be customized."

I have added a point before this, where it mentions that there will be two types of component.

oeps/oep-0028-content-theming-in-react.rst

bradenmacdonald · 2018-08-24T18:37:46Z

oeps/oep-0028-content-theming-in-react.rst

+
+* We will provide support for a global redux store which will be Open edX global redux store. This will act as a central place to store data and will have access to all data available to the system.
+
+* We will consider the layout of the data in the Open edX global redux store as a stable API. We will provide support to pre-fill the store with some common data like current user, current course, list of courses enrolled, etc. We will provide the choice for themes to fetch data that's not part of the Open edX global redux store from REST API's using built-in redux actions and store it in their own separate redux store. We will announce breaking changes if the layout of the data changes in global store.


The redux store would be specific to each frontend (LMS, Studio, ecommerce, etc.) and not global to all of "Open edX".

using built-in redux actions and store it in their own separate redux store

Actions tend to be fairly coupled to the reducers and overall store state, so I'm not sure how useful it would be to provide standard action [creator]s but expect them to work with custom stores/reducers. It might be better to just provide a standard API client library for the REST APIs, without redux actions, and let each custom theme that needs to use them include its own redux actions/reducers/state. But maybe this would help reduce boilerplate etc., and most any reducer could be adapter to ingest standard actions. Curious to know what others think.

I don't think we can expect that there will be 100% coverage for each API in the frontend, so people will have to create their own actions. I don't think it's reasonable to have the actions and reducers etc for data not used by the default theme. Where a built-in action exists, chances are that data is already loaded, and if not, the standard actions can perhaps be used.

While an API client could be automatically generated using Swagger or such, I think there should probably be multiple API clients for multiple sets of APIs because the API is huge so why should all that code be loaded if most of it isn't used. If a custom theme uses some API that the default theme doesn't, it can also include the relevant API client. This probably should cover the whole API.

@xitij2000 @bradenmacdonald I have updated this point but I am not sure if it covers your concern. Please let me know your thoughts on this. Thanks.

bradenmacdonald · 2018-08-24T18:39:32Z

oeps/oep-0028-content-theming-in-react.rst

+
+* We will use containers [2] to access data from the redux store and provide it to components via props. A container is a react component that has a direct connection to the state managed by redux and access data from the state via mapStateToProps. We will use Container as a mechanism to separate data access functionality from the Component. This way we can keep both non redux connected version as well as redux connected version of the same component.
+
+* We will have support to convert any component into a container if it needs to access any data from the redux store, which it currently does not have access to. We can see this by an example where NavbarHeader component initially displays site title. This component now needs to display authenticated username, which is there in the redux store.


I feel like the example below should be showing a NavbarHeader component that doesn't use the username, and an inherited version that does. The example here doesn't work without the container, so it isn't an example of "converting any component into a container".

I have updated this to include NavbarHeaderWithUserName which extends from NavbarHeader but completely overrides its render function. The extension will help in propagating other functionalities from NavbarHeader to NavbarHeaderWithUserName.
NavbarHeader contains support to display username which is overridden by a container. This also brings us to a point where sometimes both the component(to display additional properties) and container needs to be overridden in case we need to display some additional data.

I also don't like the idea of "converting" we are not really converting anything, we are just adding another component that passes some props by default by fetching that data from the store. So it's standard component composition, you're wrapping an existing component with another.

@xitij2000 : I have updated the "converting" part to "composition" now. Thanks:)

oeps/oep-0028-content-theming-in-react.rst

…f components.

taranjeet · 2018-08-25T09:18:34Z

@mduboseedx : Thanks. Sure I will ping you when the proposal is ready for an edx review. :)

xitij2000 · 2018-08-25T09:57:57Z

oeps/oep-0028-content-theming-in-react.rst

+        );
+    }
+
+* We will build the complete UI out of the above small modular components. We will generally use composition to build a customizable component. However, inheritance can be used at times. These customizable components will neither contain HTML nor logic. We can see an example of a customizable component called the _Header component, which is formed by the composition of SiteLogo, MainNav and UserAvatar component.


I think here the language should be "we will aim/prefer to use composition to build components wherever possible" and leave out the bits about inheritance. Instead, explain the difference in approach via an example.

I think we should also explain why this decision was made. Why prefer composition over inheritance?

I agree. What I have done is added about this in the other point and given an example of both cases. Thanks :)

oeps/oep-0028-content-theming-in-react.rst

xitij2000 · 2018-08-25T10:06:03Z

oeps/oep-0028-content-theming-in-react.rst

+.. code-block:: js
+
+    // SiteLogo being updated in Header
+    class MyThemedHeader extends _Header {


As @bradenmacdonald mentioned elsewhere, this is using inheritance not composition.

Perhaps I think you are talking about this comment. That was for another point wherein the point I talked about using "composition" but gave an example of ~~"composition"~~ "inheritance". I updated it then only.

Also here it makes sense to use "inheritance" only since if we use composition we would end up creating components like BaseHeader, Header, SiteLogoHeader and MyCustomAnimatedLogoWidgetHeader.

Let me know if this is not right.

My point is that you mention in the paragraph: "We will prefer composition whenever adding any new component or content and inheritance when replacing or modifying the component." Immediately after that, you are showing an example of using inheritance when that is not the preferred approach. Show both ways of doing this, so that it's clearer which is the preferred approach in what circumstance.

I would also say this is not a good example of inheritance at all. You are extending an existing component, but there is nothing from the original component that you are actually using. Why couldn't this just be a new component? The only thing it gets from _Header is the getNavLinks getter method and that isn't even being used, the render method is entirely replaced.

Also, I think if a component has a good chance of needing modification in some area, there should probably be a hook for it. So, in this case, replacing the Logo in the _Header is a pretty obvious modification, so perhaps there should already be a prop for it, and should just default to the standard logo.

@xitij2000 : I am not sure here but I think we should skip this difference part between composition and inheritance since that is something more specific to React and not this proposal in general. The line where we can say that here inheritance or composition which one should be used is very blur.

Keeping this in mind, I think its better to just write via composition/inheritance and leave it to the one who is implementing it because he/she will be a better judge of the situation.

I have also commented on the ticket about how we can use composition for the Header component but that turned out to be a discussion on inheritance vs composition.

class Header extends React.PureComponent { public render() { return ( <header> {props.children} </header> ); } } const BaseHeader = (props) => { <Header> {props.children} <MainNav/> <UserAvatar/> </Header> } const SiteLogoHeader = (props) => { <BaseHeader> <SiteLogo /> </BaseHeader> } const MyCustomAnimatedLogoHeader = (props) => { <BaseHeader> <MyCustomAnimatedLogo /> </BaseHeader> }

I think Braden can help us here.
@bradenmacdonald : Your inputs please :)

I think it's important to have some conventions in mind because otherwise, things can get messy. It will decide what kinds of hooks a component will provide for instance. If you're expecting composition you'd want to provide more props to control parts of the component. In the case of inheritance, you'll want methods that can be overridden etc.

I think the React ecosystem is generally more aligned with composition as you suggested, so we should favour that, and this is precisely why it becomes important to explore under what conditions would inheritance be necessary. If it never is, we should just say that.

I believe that inheritance will be necessary any time you want to remove or replace an existing component child such as the SiteLogo in the example above, unless there is some other hook for doing so via props (like logoOverride=<MyCustomAnimatedLogo />) or modifying a global (like replacing a component globally via const SiteLogo = MyCustomSiteLogo; // not StandardSiteLogo;)

@bradenmacdonald How will inheritance help in that case though? I might be missing something but here is how I see it:
Case: You have a component where the logo should be removable/replaceable.

Provide a prop that allows replacing the logo. e.g. <div> {this.props.logo === undefined?<DefaultLogo />: this.props.logo}</div>. This allows setting the logo as null or providing a custom logo without inheritance.

You inherit from the original component. But now you're overriding the original component's render method. So what exactly do you gain by inheriting vs creating a new component? The use for inheritance would be with components that have a lot of logic that will be inherited, but customisable components won't have that.

This allows setting the logo as null or providing a custom logo without inheritance.

Yes, but that assumes that the person who wrote the original component knew that people might want to override the logo. I'm sure there will be lots of other cases where the original component author didn't know what parts people might want to hide/replace/insert. So sometimes I think inheritance will be necessary.

But now you're overriding the original component's render method. So what exactly do you gain by inheriting vs creating a new component?

My original idea was that the render() methods of customizable components should be so straightforward that they are easy to override completely, in order to give the theme complete control over removing, re-ordering, replacing, or inserting children. The logic wouldn't be directly inherited, but because the child components encapsulate the relevant logic, I think it would be pretty powerful.

Maybe we should come up with more concrete examples to figure this out better? It's still fairly theoretical at this point.

Maybe we should come up with more concrete examples to figure this out better? It's still fairly theoretical at this point.

Yup, I'm just having trouble seeing a component that has a such a straightforward render function, but it still makes sense to inherit, vs create a new component. I can see the case if there is some lifecycle code or other kind of complex code in the component, but I assumed that customisable components wouldn't have that.

I agree with @xitij2000 -- inheriting from the default _Header component in this example is confusing since it doesn't accomplish anything and the result is equivalent to class MyThemedHeader extends React.PureComponent { ... }.

If we want to show a case where inheritance is useful we need a better example.

oeps/oep-0028-content-theming-in-react.rst

xitij2000 · 2018-08-25T10:23:43Z

oeps/oep-0028-content-theming-in-react.rst

+
+* We will provide support for a global redux store which will be Open edX global redux store. This will act as a central place to store data and will have access to all data available to the system.
+
+* We will consider the layout of the data in the Open edX global redux store as a stable API. We will provide support to pre-fill the store with some common data like current user, current course, list of courses enrolled, etc. We will provide the choice for themes to fetch data that's not part of the Open edX global redux store from REST API's using built-in redux actions and store it in their own separate redux store. We will announce breaking changes if the layout of the data changes in global store.


I don't think we can expect that there will be 100% coverage for each API in the frontend, so people will have to create their own actions. I don't think it's reasonable to have the actions and reducers etc for data not used by the default theme. Where a built-in action exists, chances are that data is already loaded, and if not, the standard actions can perhaps be used.

While an API client could be automatically generated using Swagger or such, I think there should probably be multiple API clients for multiple sets of APIs because the API is huge so why should all that code be loaded if most of it isn't used. If a custom theme uses some API that the default theme doesn't, it can also include the relevant API client. This probably should cover the whole API.

oeps/oep-0028-content-theming-in-react.rst

xitij2000 · 2018-08-25T10:29:13Z

oeps/oep-0028-content-theming-in-react.rst

+
+* We will use containers [2] to access data from the redux store and provide it to components via props. A container is a react component that has a direct connection to the state managed by redux and access data from the state via mapStateToProps. We will use Container as a mechanism to separate data access functionality from the Component. This way we can keep both non redux connected version as well as redux connected version of the same component.
+
+* We will have support to convert any component into a container if it needs to access any data from the redux store, which it currently does not have access to. We can see this by an example where NavbarHeader component initially displays site title. This component now needs to display authenticated username, which is there in the redux store.


I also don't like the idea of "converting" we are not really converting anything, we are just adding another component that passes some props by default by fetching that data from the store. So it's standard component composition, you're wrapping an existing component with another.

taranjeet · 2018-08-26T06:42:09Z

@xitij2000 @bradenmacdonald This is ready for your review. Thanks :)

mtyaka · 2018-11-07T11:56:42Z

Sounds good @nasthagiri, thank you for the information. Nice to hear the Architecture team is including theming in their Roadmap soon :)

natabene · 2019-02-21T18:15:30Z

@nasthagiri @ormsbee Is this a good time to return to this PR or not yet?

nasthagiri · 2019-02-22T01:39:17Z

@natabene We are aware of these 2 theming-related proposals. We plan to address them as part of our replatforming runway. As indicated on our Roadmap Progress, we are currently focusing on our "Design System" (library of reusable components). We plan to address theming only after that - but still in the 1st half of this year as previously indicated.

nasthagiri · 2019-02-22T01:48:13Z

Also, for any of you following this PR, please contact me (via slack/email) if you'd like to join an Open edX "Theming Advisory Group". We plan to consult this group on requirements and thoughts on "theming" on the Open edX platform. Thanks.

natabene · 2019-02-22T15:20:28Z

@nasthagiri Thank you for the update, super helpful to me.

mtyaka · 2019-11-06T07:30:08Z

@nasthagiri I don't have access to the replatforming runway or roadmap progress docs.

Has there been any progress on made on theming?

mtyaka · 2020-01-13T07:29:24Z

@nasthagiri There has been some exciting development around micro-frontends in the past year. Is there any documentation or guidelines on how how to theme micro-frontends?

nasthagiri · 2020-09-04T02:07:57Z

@bradenmacdonald Are there elements of this proposed OEP that you believe are still valid given the latest developments in Open edX MFEs? If not, should we close this old Draft and reopen with the latest considerations?

bradenmacdonald · 2020-09-04T22:28:07Z

@nasthagiri My understanding is that newer guidance is here in extension_points.rst#Theming Microfrontends, and specifically that the proposals in this OEP would instead be replaced by "Frontend Plugins" or entire custom MFEs (as well as Overriding Brand Specific Elements).

AFAIK frontend plugins still needs to be defined, but there's a proposal at openedx/frontend-app-discussions#5

I think it makes sense to close this and re-open when there's a solid, tested frontend plugins approach that we're ready to recommend more widely across MFEs.

nasthagiri · 2020-09-04T22:55:19Z

Thanks, @bradenmacdonald. I agree that the various efforts you enumerated will collectively form a basis for the extensibility and configurability of our frontends. Once those efforts are matured enough past their proof-of-concept phases (in ADR forms), we can create/update a frontend OEP that provides a cohesive narrative.

Given that, I will close this PR. I'm looking forward to seeing these other efforts move forward.

Cc: @abutterworth @davidjoy

openedx-webhooks · 2020-09-04T22:55:31Z

@taranjeet Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

taranjeet added 13 commits August 24, 2018 00:06

feat: Add OEP 28: Content Theming in React.

3d8dc30

chore: Fix grammatical bugs.

937040e

Update proposal with Decisions and Author name.

cfc357a

Update draft with grammatical fixes.

3b9bd8b

Add point about layout of data in redux store.

0693d12

Add Abstract and update Context.

3b015ee

Add Consequences, Arbiter and Update Last modified.

6a9a4c9

Add author and remove template comment.

12385ce

Update Context and remove typescript support.

27b002f

Update small components point under Decision.

232dd23

Update Customizable component point and example.

94bffc8

Explain container in more detail.

aa8cc5c

explain about global redux store.

dcece7f

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Aug 23, 2018

Update review period and last modified.

989ae61

taranjeet mentioned this pull request Aug 23, 2018

OEP 0028: Content theming in React taranjeet/open-edx-proposals#1

Closed

bradenmacdonald reviewed Aug 24, 2018

View reviewed changes

taranjeet added 2 commits August 25, 2018 09:37

Remove arbiter.

5ac1968

Address review comments: Remove Arbiter, update typos and add types o…

7661df7

…f components.

xitij2000 reviewed Aug 25, 2018

View reviewed changes

taranjeet added 3 commits August 26, 2018 11:00

Update language structuring.

f585d5e

Explain when to use inheritance and composition.

7b461aa

Update info about redux actions.

414759f

taranjeet added 2 commits August 26, 2018 13:08

Use React fragments.

a48125e

Dont use container directly.

4909c02

natabene removed the needs triage label Nov 7, 2018

pkulkark mentioned this pull request Feb 11, 2019

Proposal: Make "Delete My Account" text configurable openedx/edx-platform#19779

Closed

natabene added the blocked by other work PR cannot be finished until other work is complete label Feb 22, 2019

openedx-webhooks added needs triage and removed blocked by other work PR cannot be finished until other work is complete labels May 21, 2019

openedx deleted a comment from openedx-webhooks May 29, 2019

nasthagiri closed this Sep 4, 2020

openedx-webhooks added blocked by other work PR cannot be finished until other work is complete rejected and removed needs triage blocked by other work PR cannot be finished until other work is complete labels Sep 4, 2020


		* We will provide support for a global redux store which will be Open edX global redux store. This will act as a central place to store data and will have access to all data available to the system.

		* We will consider the layout of the data in the Open edX global redux store as a stable API. We will provide support to pre-fill the store with some common data like current user, current course, list of courses enrolled, etc. We will provide the choice for themes to fetch data that's not part of the Open edX global redux store from REST API's using built-in redux actions and store it in their own separate redux store. We will announce breaking changes if the layout of the data changes in global store.


		* We will use containers [2] to access data from the redux store and provide it to components via props. A container is a react component that has a direct connection to the state managed by redux and access data from the state via mapStateToProps. We will use Container as a mechanism to separate data access functionality from the Component. This way we can keep both non redux connected version as well as redux connected version of the same component.

		* We will have support to convert any component into a container if it needs to access any data from the redux store, which it currently does not have access to. We can see this by an example where NavbarHeader component initially displays site title. This component now needs to display authenticated username, which is there in the redux store.

OEP 0028: Content theming in React (Draft) #79

OEP 0028: Content theming in React (Draft) #79

Conversation

taranjeet commented Aug 23, 2018 • edited Loading

openedx-webhooks commented Aug 23, 2018

mduboseedx commented Aug 24, 2018

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

taranjeet commented Aug 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taranjeet Aug 26, 2018 • 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

taranjeet commented Aug 26, 2018

mtyaka commented Nov 7, 2018

natabene commented Feb 21, 2019

nasthagiri commented Feb 22, 2019

nasthagiri commented Feb 22, 2019

natabene commented Feb 22, 2019

mtyaka commented Nov 6, 2019

mtyaka commented Jan 13, 2020

nasthagiri commented Sep 4, 2020

bradenmacdonald commented Sep 4, 2020

nasthagiri commented Sep 4, 2020

openedx-webhooks commented Sep 4, 2020

taranjeet commented Aug 23, 2018 •

edited

Loading

taranjeet Aug 26, 2018 •

edited

Loading