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

Changes 24: Refactor the way content is passed and handled in model views #6678

Conversation

bastianallgeier
Copy link
Member

@bastianallgeier bastianallgeier commented Sep 16, 2024

Description

Summary of changes

  • content is passed as prop to the k-sections component, which will then pass it on to all child section components.
  • input and submit events are bubbling up from the fields section -> to the sections component -> to the model view components.
  • The k-form-button component is simplified and no longer handles submit and discard events itself. It sends the events up to the model view components to be handled there.
  • The file preview components now also receive the content as property and can send input and submit events up to the model view.
  • The ModelView component class takes over handling submit, input and discard events. It also listens to the view.save event and handles that. With this change, each view component is the single place of truth to pass content down to child components and handle input, submit and discard events. This drastically reduces the mental load to parse which component is responsible for what and the child components can be a lot more simple.

Reasoning

Simplifying the content flow will help a lot with the next steps when changes are coming from the backend. We can pass them as view property and the view can then pass it further down. This will avoid having to run some weird content module init code like we currently do in the Vuex module. It will also no longer tightly couple module logic with the components.

Next steps

  • The k-form-buttons component is about to be removed and replaced by the new k-form-controls component. I wanted to refactor it in this PR anyway, to already get prepared for this step. The form controls component will also send the events up to the model view so it makes sense to already do that and reduce the dependency on the old form buttons component.
  • The content module will likely need a few more changes. I've prepared a couple things on the next-steps branch, but didn't want to blow this PR up even more.

@bastianallgeier bastianallgeier marked this pull request as ready for review September 16, 2024 14:49
@distantnative
Copy link
Member

I think the consistency of this PR is a big pro argument for it. I am still a bit hesitant whether leaning into these huge chains (bubbling all the way up and down) is the best way. We have seen in the past how it can lead to hard debug bugs based on latency that this adds.

There are parts where it makes a lot of sense, but others like the image file preview component where we have to route stuff upward and downwards multiple layers, where I think interacting directly with the content module might be the better approach.

My largest concern is not a change in this PR, but what it prepares and you mention in the description: Content and changes being passed as view props. While this will work nicely for the chain of props of this PR, if the content module is not the main host of that information how can a field get the value of another field? How can developers interact with other data. I think putting them into props of a component binds that data way too much to a component (the model view here) than it should.

My counter idea would be that the data is core to the content module and gets passed from the backend via Fiber to the module, just as all other panel JS modules update their state from the backend. This probably also gets rid of all the weird init calls from components, but would establish the content module a lot more as the central truth and actor for anything content-related.

@bastianallgeier
Copy link
Member Author

That's interesting. I never really thought about performance issues with those bubble chains. My assumption was that the traditional Vue pattern of handling down props and sending up events is actually the fastest solution. When we avoided the chains, we mostly did it because it blows up complexity in some cases. But here, it really didn't feel like a big deal to go down 3 levels

ModelView -> Sections -> FieldsSection

Our biggest performance issues with fields have always been the changes comparison so far – at least that's what I remembered. But I could indeed be very wrong here, which would definitely speak for a different route.

The second point about the main host of the content is a bit unclear in this PR and I should have explained the next steps better here.

In the next-steps branch, I have panel.view.props.content and panel.view.props.originals.

panel.view.props.originals will always have an unchanged object of the latest published version. panel.view.props.contentwill have a full object of changed and unchanged fields. This way, we have a really nice option to compare both objects with each other to detect changes.

Both come directly from the backend and are being passed to the ModelView component and from there to k-sections. k-sections will pass the content prop to each section and the section component can then decide if it actually needs it or not. This way, not only the fields section, but any other section types can directly read the content property if needed. They can also send input and submit events to update the content.

Your content.js module still exists to handle the API requests (save, discard) and provide some convenient access methods for content. But the content.js module directly uses the content and original view props above.

This will give us a similar setup to dialogs and dawers. We can modify the reactive view props objects via the content.js moduel and those will trigger component updates in the model view components and all their child components correctly.

In my tests on the next branch, this felt like a really robust setup.

@bastianallgeier bastianallgeier changed the title Refactor the way content is passed and handled in model views Changes 24: Refactor the way content is passed and handled in model views Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants