Temp8 Fix for #4011 #4560
Temp8 Fix for #4011 #4560
Conversation
ghost
assigned 
There was a problem hiding this comment.
@nielslyngsoe Please remember to review your changes when submitting PRs, we should not be shipping console.log or commented out code, there are a bunch of places in this PR where this exists.
Also if you can please describe some testing notes since I'm really unsure about what this is fixing and what I'm supposed to be testing. I understand it helps fix #4011 but what else?
src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js
Outdated
Show resolved
Hide resolved
|
It almost works as expected. But the
|
|
Hi @bergmania, please review again. I have implemented the feedback and made a solution to the problems with Embed and Media. |
|
@nielslyngsoe, the changes fixes the issues :) |
There was a problem hiding this comment.
I can verify that the #4011 issue is resolved but i still don't understand all of the changes in this PR.
Like my other comment suggests (see #4560 (review)) we need testing notes, how do I know all of these changes are doing what they are supposed to do?
There are other changes in here that don't relate to the original issue too, please try keeping PRs limited to specific scope otherwise it's hard to know what to test.
I've left notes in here about binding to methods in angular which we should avoid.
src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js
Outdated
Show resolved
Hide resolved
|
Please see latest version for corrections of the PR based on the comments. |
# Conflicts: # src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js
| border-style: dashed; | ||
| border-color: @gray-8; | ||
| border-color: @ui-action-disgrete-border; |
There was a problem hiding this comment.
This word disgrete is showing up again in quite a few places in this PR. This is also in the main repo too, see https://github.com/umbraco/Umbraco-CMS/search?q=disgrete&unscoped_q=disgrete
Can we fix this up to be discrete (assuming that's what it supposed to be?)
| @@ -1,10 +1,10 @@ | |||
| <div ng-controller="Umbraco.PropertyEditors.Grid.EmbedController"> | |||
|
|
|||
| <div class="umb-editor-placeholder" ng-click="setEmbed()" ng-if="trustedValue === null"> | |||
| <div class="umb-editor-placeholder" ng-click="setEmbed()" ng-if="hasEmbed() === false"> | |||
There was a problem hiding this comment.
This should not be a function call, it should be control.value !== null, function calls in views are expensive
There was a problem hiding this comment.
Turns out, it has to be control.value === null. The hasEmbed is negated.
…es is invariant. Before this change an exception was thrown
# Conflicts: # src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js # src/Umbraco.Web.UI.Client/src/views/components/grid/grid-rte.html # src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js # src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.html
|
The "disgrete" vs "discrete" is fixed and merged here: #4961 |
Cleaned up ui-state variables so they are not part of data.
Plus fixed #4011