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

Improvements for the content.js module #6799

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

bastianallgeier
Copy link
Member

@bastianallgeier bastianallgeier commented Nov 18, 2024

Description

Summary of changes

  • New option to pass the silent argument to api requests in the options object. This will simplify the API calls quite a bit. Before: this.$panel.api.post('/path', data, options, 'POST', silent = true) after: this.$panel.api.post('/path', data, { silent: true}) This is now used in this.$panel.content.save() to keep safe calls silent.
  • New changes property for the k-model-tabs component to pass an object of changes directly. This will make the model tabs more independent from the content module and we can reuse the computed changes object that we already define in the model views.
  • All content module methods with a dependency on the api endpoint now have a second optional api endpoint argument. This argument will use the view's api prop by default.
    • this.$panel.content.discard(api)
    • this.$panel.content.isLocked(api)
    • this.$panel.content.lock(api)
    • this.$panel.content.publish(values, api)
    • this.$panel.content.save(values, api)
    • this.$panel.content.saveLazy(values, api)
    • this.$panel.content.update(values, api)
  • All content module methods will throw an Error if they cannot be used on a different view for a different API endpoint
  • New content module methods:
    • this.$panel.content.isLocked()
    • this.$panel.content.lock()
    • this.$panel.content.changes()
    • this.$panel.content.isCurrent()
    • this.$panel.content.saveLazy()
  • Removed content module props/getters
    • this.$panel.content.isSaved
    • this.$panel.content.originals
  • this.$panel.content.saveLazy is now used in the model view onInput event instead of throttling it directly in the view. This means that devs can now choose between calling this.$panel.content.save or this.$panel.content.saveLazy if the action should be delayed.
  • The ModelView onSave method has been renamed to onViewSave This matches the custom event in the events.js, which will trigger this and causes less confusion. This event is only fired on keydown.cmd.s and will in fact submit the form and not simply save the content. That's why I believe we should rename this right away.
  • The ModelView isUnsaved computed property has been renamed to hasChanges. This is also supposed to clean up possible confusions, because it's computing the number of items in the this.$panel.content.changes() object.
  • All actions in the content module now fire custom events after the requests went through:
    • content.save
    • content.discard
    • content.publish
  • The new content.save event is used to update the new ModelView isSaved data property. By moving it to the ModelView and using the custom event to keep it updated, we can make sure that it is always correct and bound to the right api endpoint. Otherwise, we would need to keep track of multiple isSaved states in the content module.
  • Redundant on methods have been removed from the PreviewView.
  • The outdated protectedFields property has been removed from ModelView

Outlook

Methods that depend on data from the current view could later be refactored to make API calls instead whenever they are used with different endpoints.

I.e. right now this.$panel.content.lock() wiil only work for the current view and throw an error if you try to pass a different endpoint. But it's totally possible to simply check for the endpoint and then make an async call to that view instead and get the lock info from there. The same would work for this.$panel.content.changes() for example. But I didn't want to blow this up even more. I just think that this is a nice way forward that could be really useful.

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

panel/src/components/Views/Files/FileView.vue Outdated Show resolved Hide resolved
panel/src/components/Views/ModelView.vue Outdated Show resolved Hide resolved
panel/src/panel/content.js Outdated Show resolved Hide resolved
panel/src/panel/content.js Outdated Show resolved Hide resolved
panel/src/panel/content.js Show resolved Hide resolved
@bastianallgeier bastianallgeier merged commit 700bbe6 into v5/changes/develop Nov 20, 2024
4 checks passed
@bastianallgeier bastianallgeier deleted the v5/changes/content branch November 20, 2024 07:43
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