Skip to content
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

Decouple editor components from state management/api endpoints #1238

Open
brian-smith-tcril opened this issue Feb 8, 2023 · 9 comments
Open

Comments

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Feb 8, 2023

Existing architecture

In its current state, the components in this repository work under the following assumptions:

  • The component will be the only thing the user is interacting with at that time (via a modal or a simulated modal)
  • The redux state within frontend-lib-content-components will exist

These assumptions become apparent when attempting to integrate the editors from this repository into an MFE, as the only exposed component is the EditorPage

To the best of my understanding, this architectural decision was made with the intention to simplify the integration of third-party editors (by adding them to supportedEditors I assume), providing all the state management out-of-the-box.

Counter-intuitivity

This architecture runs counter to the expected behavior of a separate repository with “components” in its name. As a consumer of Editor components, I would expect to be able to:

  • Place the component where I see fit, be that in a modal, or in an existing tab on a page.
  • Provide my own state management.

Proposed refactor

I would like to propose:

  • A refactor that decouples the components from the redux store, allowing consumers to utilize external state management. This includes intra-component controls, hooks, or events that have external effects (such as a "save" button), which should be handled by functions passed in as properties so that no API endpoints need be hard-coded into the components
  • If necessary, existing editor components be made compatible with v2 library blocks
  • The components be exported in a way that allows them to exist outside of a single page/modal editing experience

With these changes in place, frontend-app-library-authoring could consume the editor components while providing it's own state management. This would also allow frontend-app-library-authoring to render the components within the existing “edit” tab, instead of reworking the UX to accommodate a modal editing experience

@connorhaugh
Copy link
Contributor

connorhaugh commented Feb 8, 2023

This architecture runs counter to the expected behavior of a separate repository with “components” in its name.

I have more nuanced concepts to share than this, but just want to cover this for clarifcation, before we get too deep into it. If this is the crux of it, how amenable are people to changing the name of this repository//package? Would that assuage your concerns with its architecture?

@brian-smith-tcril
Copy link
Contributor Author

how amenable are people to changing the name of this repository//package? Would that assuage your concerns with its architecture?

While this would address the counter-intuitivity problem, it would not address the flexibility concerns.

  • The editors would still only be usable by pages that expect a modal.
  • The editors would still not be able to utilize external state management.

Ideally, we would have editor components that can be used in multiple contexts.

One possible way to handle this would be to:

  • Pull the editor components out of this repo, make them flexible enough to work in multiple contexts
  • Use this repo to provide context/state management to the flexible editor components

Consuming applications would then have the option to either:

  • Consume EditorPages from this repo, utilizing the supported/included state management
  • Consume Editors directly from a different repo, providing their own context/state management

@connorhaugh
Copy link
Contributor

Hi Brian,

I think that proper interrogation of architecture cannot be done by navel gazing alone, so this is certainly helpful!

I think your question reveals something: upon better defining terms, you will see that this repo holds as core tenants the separation you are looking for, we just need to better outline and point to this separation.

I want to start by defining terms:

“The Framework Layer” refers to a layer which manages the editing lifecycle. It engages with the xblock api to retrieve and send information, manages its state using a redux slice, and provides visual components for the modal, exiting and entering.

“The Editor Layer” refers to the layer which manages the editor itself. It also has its own redux state. It exposes a getContent callback function (which computes the editor’s current value), and takes in a myriad of props, including, but not limited to: the xblock’s data (metadata, studio view), assets associated with the block or its learning context, and the course’s settings.

All of these props are presently provided using a connected component. With some minor refactoring, these “editor layer” components could be exported separately, and allow developers to use the editors outside of the modal context. Just to be clear:

  1. We don’t want a refactor from an outside team on features we are actively developing. It will be best to do a refactor which better separates the code after we have finished inserting the editors into Library authoring in the modal pattern our designs suggest.

  2. The “framework layer” will use redux for state management and encompass api endpoints/calls. The “editor” layer will continue to use redux to manage its internal state.

  3. However, we are very interested in better defining the boundaries between the “editor” and “framework” layers. The “framework” should only provide props to the editors. The components could then be exported in a way that allows them to exist outside of a single page/modal editing experience. To do so, some code cleanup is required, but more importantly, we need to better document the architecture of the repo and define and expose the line between “editor” and “framework.”

One important final question is: what are the specific goals you are currently trying to accomplish with such a refactor, but cannot now that I have stated our intentions in this way?

Connor & TNL

@connorhaugh
Copy link
Contributor

connorhaugh commented Feb 10, 2023

Hi Brian -- I think you'll notice your most recent response occurred as we were conferring on ours. I think we do have a lot of consensus, and I think we will definitely have to talk more about "what goes in this repo." I am more inclined to include both editors and framework in this repository, but just export both layers. I'm open to reasons why this would be a bad idea, but I think making two repos when one will do only adds complexity.

@brian-smith-tcril
Copy link
Contributor Author

It definitely feels like we have quite a bit of consensus. I'll need a little bit of time to put my thoughts together on the larger Framework vs. Editor layer stuff. The question of "how flexible is flexible enough" comes to mind when thinking about the Editor layer, but I haven't fully thought that through.

I do have thoughts ready on (what feels like) the smaller issue of 1 vs 2 repos:

I am more inclined to include both editors and framework in this repository, but just export both layers.

I'm not opposed to this idea. As long as the editor layer can be consumed without the framework layer one repo should be fine.

I'm open to reasons why this would be a bad idea, but I think making two repos when one will do only adds complexity.

The only concern here is that we might end up circling back around to the counter-intuitivity problem. Consuming the editor layer from something named frontend-lib-content-components feels intuitive. Consuming the framework layer from something named frontend-lib-content-components feels a little more odd.

That being said, this is a minor issue at most. Really just an instance of the second hard thing in programming (naming things).

@kenclary
Copy link
Contributor

Concerning naming the repo (and only that)...

The original vision and name for the repo used a more-liberal definition of 'component' that is being used here. Perhaps that was a mistake.

Personally, I would not want to see this broken up into multiple repos --- indeed, that would be directly counter to the original intention. Maybe a simple name like frontend-lib-content or something like frontend-lib-content-tools would be more accurate, if we refuse to use the broad definition of 'component.'

@jesperhodge
Copy link
Member

jesperhodge commented Feb 28, 2023

frontend-lib-content-editors?

@jesperhodge
Copy link
Member

Hi @brian-smith-tcril , we haven't forgotten about this issue. The separating of framework and editor layers is still something we desire, and another issue that Connor created for changing the folder structure comes into play here. You can follow along here. We will have rather slow progress on that for the time being because there are multiple considerations to account for, and we need to take time for conversations around that. Nevertheless we are talking about these folder structure changes right now, with the concrete goal to implement changes when ready, and I will take care of steadily moving this forward.

@bradenmacdonald
Copy link
Contributor

Hi folks, I'm just wondering if this is something people are still interested in or planning to work on?

Context: I'm starting work on integrating these editors into the new content libraries UI, and I've noticed these same issues, as well as the problems pointed out in the discussion about code organization. I also have a bunch of other simplifications and cleanups I'd like to make, listed in openedx/frontend-lib-content-components#499

@bradenmacdonald bradenmacdonald transferred this issue from openedx/frontend-lib-content-components Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

6 participants