Skip to content
This repository was archived by the owner on Jun 18, 2018. It is now read-only.

Fix grid editors in nested content #116

Closed
wants to merge 3 commits into from

Conversation

SudoCat
Copy link

@SudoCat SudoCat commented Mar 24, 2017

This would resolve #109. I can't say it's the most eloquent solution, but it does allow you to have grid editors inside of nested content, which allows for incredibly powerful layouts.

Without this fix, the grid config is lost on close due to sending the formSubmitted signal to the grid.

  • Backup and restore grid config when closing editor containing grids.
  • Swap ng-if for ng-show to avoid re-initing the RTE. Not ideal, but does work.

Ideally, this should use ng-if and reinitialise any rich fields on show. It would also be preferable to reacquire the grid config on open, rather than simply storing it; However, I'm just stoked to get it working!

- Backup and restore grid config when closing editor containing grids.
- Swap ng-if for ng-show to avoid re-initing the RTE. Not ideal, but does work.
}

var backupGridConfig = function (prop) {
return prop.editor == "Umbraco.Grid" ? prop : false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really want a hard dependency on another property-editor, the idea of Nested Content is to be agnostic.

@mattbrailsford - we could look at options to have extensibility points? events, callbacks?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally agree, we should not be tying nested content to the grid property editor so some other solution is needed. I'm not sure what we can do with events from the angular side though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair enough, I thought it only seemed right to try and support as many of the native Umbraco property types as possible, and I couldn't see any other way to cater for the grid - Ideally you would just trigger all of the network requests again on open, but I'm not enough of an Angular or Umbraco genius to figure that one out - at least not on company time haha.

@@ -25,7 +25,7 @@

</div>

<div class="nested-content__content" ng-if="$parent.realCurrentNode.id == node.id && !$parent.sorting">
<div class="nested-content__content" ng-show="$parent.realCurrentNode.id == node.id && !$parent.sorting">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd already switched from using ng-show to ng-if, there was a performance impact with ng-show when dealing with larger lists. See comments in PR #40 and commit 6180448

@leekelleher
Copy link
Collaborator

@SudoCat Thanks for the PR! 👍 I've added some comments, but will review further with @mattbrailsford.

@SudoCat
Copy link
Author

SudoCat commented Mar 27, 2017

I've updated this branch following your comments. You were very right that Nested Content shouldn't include a hard coded fix for another specific property editor, and nor should ng-show be used.

Taking this into consideration, I have removed the previous technique, and I have instead created an angular service which can be used to create an extendable callback system, which works very similarly to $broadcast. Right now, I have only added in one callback, as I wasn't sure where any others might be useful - if at all.

The callback service can be easily extended through the use of angular decorators. Here is an example of the decorator I have created to solve my Grid Property Editor (Apologies for the ES6 - it's just much faster when I'm prototyping).

angular.module('umbraco').config([

  '$provide',

  function($provide) {

    $provide.decorator('Our.Umbraco.NestedContent.Services.NestedContentCallbacks', [
      '$delegate',
      function $logDecorator($delegate) {

        // Define function to use as callback
        function ncGridCallback(args) {

          args.scope.tab.properties.forEach(prop => {
            prop.editor != "Umbraco.Grid" || prop.value.sections.forEach(section => section.rows.forEach(row => {
              row.$initialized = false
              row.areas.forEach(area => area.controls.forEach(control => control.$editorPath = null))
            }) );
          })

        }

        // Make sure callback exists, then append function to callback array.
        if ( $delegate.callbacks.ncBeforeFormSubmitting && Array.isArray($delegate.callbacks.ncBeforeFormSubmitting) ) {
          $delegate.callbacks.ncBeforeFormSubmitting.push(ncGridCallback);
        }

        return $delegate;
      }
    ]);
  }
]);

Instead of caching and replacing the grid config, this technique instead resets the grid config when you close the nested content, causing the entire grid to be re-rendered on open - resulting is a much smoother UX with - as of yet - no weird bugs!

Again, I know this is quite an edge case issue, but I think that a callback system could be indispensable for 3rd party extensions.

Copy link
Collaborator

@mattbrailsford mattbrailsford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not averse to this as it doesn't add much code to NC and it does remove that PE dependency.

My only comment would be, given we require them to use decorators to extend the behavior, wouldn't it be easier for the service to just have a "callback" method, and devs use the decorator to extend/wrap this callback method? ie, there is no need to keep track of the callbacks, the decorator will just wrap the callback method and perform it's code as well.

Another alternative is we could actually use events like the rest of the code does, so someone could register a listener and do the same thing. Is there any reason this approach isn't used? purely from a consistency POV.

@SudoCat
Copy link
Author

SudoCat commented May 10, 2017

Thanks for getting back to me on this.

I'm not sure I entirely understand - do you mean to have a single callback method which can be overwritten? I went with the object of arrays mostly for simplicity. It provided a quick and easy way to build a versatile callback system with room to easily add new callback events and multiple methods for each one without developers accidentally overwriting one another's callbacks. Within the constraints of my limited angular knowledge, this seemed like a fairly reasonable approach. Apologies if I've misunderstood what you meant.

I have looked into using events with $scope.$on and $scope.$broadcast, but as far as I could tell, you can't bind an event to the scope without being in a controller/directive, which require markup changes to initialise. This didn't seem practical for 3rd parties trying to extend the callbacks. I could well be mistaken though, as I don't know angular that well.

Right now to get this functionality working in development without editing Nested Content and preventing updates, I have a provider overwriting the nestedContentEditorDirective functionality. This ended up requiring an a callback to fire before and after the formSubmitting event. Hopefully this helps explain why I chose the callback method I did.

Here's a gist with my latest implementation

@leekelleher leekelleher added this to the Umbraco 7.7.x milestone Aug 23, 2017
@leekelleher
Copy link
Collaborator

Closing the PR, as we don't want to introduce any features that are different to the version we migrated to (upcoming) Umbraco v7.7.0.

If you still want this feature in a future Umbraco release, please feel free to raise this with the Umbraco core team via their issue tracker.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants