editor: fix issues #338

jma · 2021-01-07T15:29:57Z

Fixes tester document search routing.
Removes useless log messages.
Fixes import paths.
Replaces the css classes such as editor-title by a card wrapper.
Moves some common code lines in the editor service.
Moves the field label html code into a specific component.
Adds new containerCSSClass, itemCSSClass and cssClass to allow
field grid positions.
Renders the hide/show/clone button in the children field instead of
the parent (array, object) component.
Adds external link support for remote typeahead editor component.
Closes Property with hide expression cannot be added #328.
Closes Hidden sub-property with hide expression #327.
Closes Read-only properties are changeable in editor #325.
Closes Additional border is displayed for object types #248.
Closes Object data are not cleaned correctly when the property is removed #242.
Closes Display optional repetitive field only once rero-ils#1604.
Closes Cannot add new fields to a duplicate bibliographic record rero-ils#1601.

Co-Authored-by: Johnny Mariéthoz [email protected]

Why are you opening this PR?

To solve several issue on SONAR.

How to test?

-Start the demo server and go to http://localhost:4200/editor.

Code review check list

sebdeleze

There's a typo in commit message: routing instead of rooting.

And you can reference the solved issues to close them automatically.

sebdeleze · 2021-01-12T08:17:06Z

projects/rero/ng-core/src/lib/record/editor/editor.component.scss

+@each $breakpoint in map-keys($grid-breakpoints) {
+  @include media-breakpoint-up($breakpoint) {
+    $infix: breakpoint-infix($breakpoint, $grid-breakpoints);
+    @each $prop, $abbrev in (width: w, height: h) {
+      @each $size, $length in $sizes {
+        .#{$abbrev}#{$infix}-#{$size} { #{$prop}: $length !important; }
+      }
+    }
+  }
+}


Be careful, I think it can create a black hole 🤣

an after that, @jma tells me that he isn't a CSS king .... What's the truth ?

It is just a copy/paste from the web.

sebdeleze · 2021-01-12T08:18:43Z

projects/rero/ng-core/src/lib/record/search/record-search.component.ts

+        // },
+        // (error) => {
+        //   this.error = error;
+        //   this._spinner.hide();
+        // }


Better to remove instead of commenting, Git have the history!

sebdeleze · 2021-01-12T08:33:32Z

projects/rero/ng-core/src/lib/record/search/record-search.component.html

@@ -30,6 +30,7 @@
    </li>
  </ul>
  <div class="main-content">
+    {{types}}


Must be removed

projects/rero/ng-core/src/lib/record/editor/services/editor.service.ts

sebdeleze · 2021-01-12T08:37:54Z

projects/rero/ng-core/src/lib/record/editor/type/array-type/array-type.component.ts

+  // /**
+  //  * Hide the array and remove all the elements
+  //  */
+  // hide() {
+  //   this.field.hide = true;
+  // }


Better to remove instead of commenting

sebdeleze · 2021-01-12T08:40:31Z

projects/rero/ng-core/src/lib/record/editor/type/multischema/multischema.component.ts

-    <div>
-      <legend *ngIf="to.label" [tooltip]="to.description">{{ to.label }} </legend>
+    <div class="{{field.parent.templateOptions.cssClass}}">
+      <legend *ngIf="to.label" [tooltip]="to.description">label {{ to.label }}</legend>


label should be removed.

sebdeleze · 2021-01-12T08:43:04Z

...ore/src/lib/record/editor/widgets/dropdown-label-editor/dropdown-label-editor.component.html

-      class="dropdown-menu"
-      role="menu"
-      aria-labelledby="button-basic">
+    <div id="dropdown-basic" *dropdownMenu class="dropdown-menu" role="menu" aria-labelledby="button-basic">


aria-labelledby should be removed as it refer to a non-existing ID.

sebdeleze · 2021-01-12T08:54:27Z

...ro/ng-core/src/lib/record/editor/wrappers/horizontal-wrapper/horizontal-wrapper.component.ts

+  constructor(private _editorService: EditorService) {
+    super();
+  }
+
+  getIndex() {


Doc block is missing for constructor and getIndex

sebdeleze · 2021-01-12T08:56:29Z

projects/rero/ng-core/src/lib/record/editor/widgets/label/label.component.ts

+  getIndex() {
+    if (this.field.parent.type === 'array') {
+      return Number(this.field.key);
+    }
+    return null;
+  }
+
+  getFieldGroup(field: FormlyFieldConfig) {


doc blocks are missing for getIndex and getFieldGroup

sebdeleze · 2021-01-12T09:18:17Z

...ro/ng-core/src/lib/record/editor/wrappers/horizontal-wrapper/horizontal-wrapper.component.ts

+  add() {
+    if (this.field.parent.type === 'array') {
+      const i = this.getIndex() + 1;
+      console.log('i:', i);


Remove console log

sebdeleze · 2021-01-12T09:31:29Z

I tested the issues and some of them seem not to be corrected:

Additional border is displayed for object types #248 —> OK
Tooltips are sometimes displayed twice #196 —> OK
Read-only properties are changeable in editor #325 —> KO (but issue open in ngx-formly)
Hidden sub-property with hide expression #327 —> KO (Work with hide but not with hideExpression)
Property with hide expression cannot be added #328 —> KO (Still cannot be added)
Hidden property with a default value is still displayed #329 —> KO (Hidden from form but not available in search and listed in jump to section)

zannkukai

Most of comments are cosmetics ;-) you can skip them

zannkukai · 2021-01-15T13:21:31Z

projects/rero/ng-core/src/lib/record/editor/services/editor.service.ts

+    if (!field.templateOptions.longMode) {
+      return false;
+    }
+    return (
+      !field.templateOptions.required &&
+      !field.hide &&
+      field.hideExpression == null
+    );


I like (me and myself) ternary instruction. In this case

return (!field.templateOptions.longMode) ? false : ( !field.templateOptions.required && !field.hide && field.hideExpression == null );

Seem to me more difficult to read. @sebastiendeleze any advises?

+1 for use ternary, but only if expression is quite simple. In this case, it is difficult to read in my opinion. But I would suggest to merge statements, like this:

return ( field.templateOptions.longMode && !field.templateOptions.required && !field.hide && field.hideExpression == null );

zannkukai · 2021-01-15T13:22:33Z

projects/rero/ng-core/src/lib/record/editor/services/editor.service.ts

+    if (!field) {
+      return false;
+    }
+    return field.templateOptions && field.templateOptions.isRoot && field.templateOptions.isRoot === true;


ternary is cosmetic ;-)

return (!field) ? false : field.templateOptions && field.templateOptions.isRoot && field.templateOptions.isRoot === true;

zannkukai · 2021-01-15T13:27:11Z

projects/rero/ng-core/src/lib/record/editor/widgets/label/label.component.html

+  <!-- section title -->
+  <ng-template #title let-f="f">
+    <div class="header">
+      <label class="mb-0" *ngIf="f.templateOptions.label" [attr.for]="f.id" [tooltip]="f.templateOptions.description">


maybe introduce the possibility to add classes on label ?
See https://github.com/rero/ng-core/pull/339/files#diff-869d70bf847d429975de17d5bba4a3b9d16c3ce727a971e147a93deaaa8d0fcdR25

I prefer the more generic approach: https://github.com/rero/ng-core/pull/338/files#diff-869d70bf847d429975de17d5bba4a3b9d16c3ce727a971e147a93deaaa8d0fcdR24

zannkukai · 2021-01-15T13:27:49Z

projects/rero/ng-core/src/lib/record/editor/widgets/label/label.component.ts

+   * @param field - FormlyFieldConfig, the correspondig form field config
+   * @returns boolean, true if the menu should be displayed
+   */
+  hasMenu(field: FormlyFieldConfig) {


zannkukai · 2021-01-15T13:28:37Z

projects/rero/ng-core/src/lib/record/editor/widgets/label/label.component.ts

+  getIndex() {
+    if (this.field.parent.type === 'array') {
+      return Number(this.field.key);
+    }
+    return null;


This one is the perfect one for a ternary return :
return (this.field.parent.type === 'array') ? Number(this.field.key) : null;

zannkukai · 2021-01-15T13:36:53Z

projects/rero/ng-core/src/lib/record/editor/widgets/label/label.component.ts

+    if (this.field.parent.type === 'array') {
+      return this.field.parent.templateOptions.canAdd();
+    }
+    return false;


zannkukai · 2021-01-15T13:37:48Z

projects/rero/ng-core/src/lib/record/editor/widgets/label/label.component.ts

+      const i = this.getIndex() + 1;
+      return this.field.parent.templateOptions.add(this.getIndex() + 1);


Either use i, either delete line 173

zannkukai · 2021-01-15T13:38:40Z

...ro/ng-core/src/lib/record/editor/wrappers/horizontal-wrapper/horizontal-wrapper.component.ts

+    if (this.field.parent.type === 'object') {
+      return this._editorService.canHide(this.field);
+    }
+    if (this.field.parent.type === 'array') {
+      return this.field.parent.templateOptions.canRemove();
+    }
+    return false;


zannkukai · 2021-01-15T13:38:51Z

...ro/ng-core/src/lib/record/editor/wrappers/horizontal-wrapper/horizontal-wrapper.component.ts

+    if (this.field.parent.type === 'array') {
+      return this.field.parent.templateOptions.canAdd();
+    }
+    return false;


ternary again ?

zannkukai · 2021-01-15T13:39:17Z

...ro/ng-core/src/lib/record/editor/wrappers/horizontal-wrapper/horizontal-wrapper.component.ts

+      const i = this.getIndex() + 1;
+      return this.field.parent.templateOptions.add(this.getIndex() + 1);


use i or delete the line 107

sebdeleze · 2021-01-19T13:17:40Z

When I tried the editor on SONAR, the main field wrapper and labels are appearing twice. I noticed this problem in ng-core-tester, too.

sebdeleze · 2021-01-19T13:42:25Z

When I try to create a new document, some fields appear both in the left side and in the editor.

sebdeleze · 2021-01-19T13:45:41Z

Based on the schema property here https://github.com/rero/sonar/blob/staging/sonar/modules/documents/jsonschemas/documents/document-v1.0.0_src.json#L313, I think the subtitles must not be present in the editor by default:

I think this is related to #338 (comment) and the hideExpression management, because when the document type is changed, the field subtitles is shown or hidden, even though it has no hideExpression property.

jma · 2021-01-20T17:06:12Z

When I tried the editor on SONAR, the main field wrapper and labels are appearing twice. I noticed this problem in ng-core-tester, too.

It should be solved

iGormilhit

Commit message approved.

* Fixes tester document search routing. * Removes useless log messages. * Fixes import paths. * Replaces the css classes such as `editor-title` by a `card` wrapper. * Moves some common code lines in the editor service. * Moves the field label html code into a specific component. * Adds new `containerCSSClass`, `itemCSSClass` and `cssClass` to allow field grid positions. * Renders the hide/show/clone button in the children field instead of the parent (array, object) component. * Adds external link support for remote typeahead editor component. * Closes rero#328. * Closes rero#327. * Closes rero#325. * Closes rero#248. * Closes rero#242. * Closes rero/rero-ils#1604. * Closes rero/rero-ils#1601. Co-Authored-by: Johnny Mariéthoz <[email protected]>

sebdeleze · 2021-01-29T11:38:17Z

projects/rero/ng-core/src/lib/record/editor/type/multischema/multischema.component.ts

@@ -14,14 +14,14 @@
 * You should have received a copy of the GNU Affero General Public License
 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
-import { Component } from '@angular/core';
+import { Component, OnInit } from '@angular/core';


On Init is not used.

jma force-pushed the maj-fix-editor-issues branch 3 times, most recently from a2e12a6 to 1a803ec Compare January 11, 2021 09:42

jma changed the title ~~Maj fix editor issues~~ editor: fix issues Jan 11, 2021

jma force-pushed the maj-fix-editor-issues branch from 1a803ec to 7bed2d2 Compare January 11, 2021 10:23

jma marked this pull request as ready for review January 11, 2021 12:22

jma requested review from sebdeleze and zannkukai January 11, 2021 12:22

sebdeleze suggested changes Jan 12, 2021

View reviewed changes

jma marked this pull request as draft January 13, 2021 20:03

jma force-pushed the maj-fix-editor-issues branch 2 times, most recently from 2601da7 to e411862 Compare January 14, 2021 14:15

jma marked this pull request as ready for review January 14, 2021 14:31

zannkukai requested changes Jan 15, 2021

View reviewed changes

jma force-pushed the maj-fix-editor-issues branch 4 times, most recently from 3f40ac9 to 51f4a19 Compare January 18, 2021 15:48

jma requested review from sebdeleze and zannkukai January 18, 2021 15:48

zannkukai approved these changes Jan 19, 2021

View reviewed changes

jma force-pushed the maj-fix-editor-issues branch 3 times, most recently from dab1625 to 75f2990 Compare January 19, 2021 20:14

jma force-pushed the maj-fix-editor-issues branch 5 times, most recently from d3a0de3 to 2955b8e Compare January 20, 2021 14:05

iGormilhit added the f: editor Concerns editor based on JSON schema AND custom editor label Jan 25, 2021

jma force-pushed the maj-fix-editor-issues branch 3 times, most recently from ba2a45f to 0529063 Compare January 27, 2021 13:14

jma mentioned this pull request Jan 27, 2021

editor: add an external link to the mef authority rero/rero-ils-ui#503

Merged

7 tasks

jma force-pushed the maj-fix-editor-issues branch from 0529063 to 3c53cb3 Compare January 27, 2021 13:47

jma mentioned this pull request Jan 27, 2021

editor: adapt JSON schema form options rero/rero-ils#1647

Merged

7 tasks

jma force-pushed the maj-fix-editor-issues branch from 3c53cb3 to f289992 Compare January 27, 2021 15:32

iGormilhit approved these changes Jan 28, 2021

View reviewed changes

jma force-pushed the maj-fix-editor-issues branch from f289992 to 4198063 Compare January 28, 2021 10:22

sebdeleze approved these changes Jan 29, 2021

View reviewed changes

jma merged commit ca9caf8 into rero:dev Feb 10, 2021

jma deleted the maj-fix-editor-issues branch December 13, 2021 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

editor: fix issues #338

editor: fix issues #338

jma commented Jan 7, 2021 •

edited

Loading

sebdeleze left a comment

sebdeleze Jan 12, 2021

zannkukai Jan 15, 2021

jma Jan 18, 2021

sebdeleze Jan 12, 2021

sebdeleze Jan 12, 2021

sebdeleze Jan 12, 2021

sebdeleze Jan 12, 2021

sebdeleze Jan 12, 2021

sebdeleze Jan 12, 2021

sebdeleze Jan 12, 2021

sebdeleze Jan 12, 2021

sebdeleze commented Jan 12, 2021

zannkukai left a comment

zannkukai Jan 15, 2021

jma Jan 18, 2021

sebdeleze Jan 19, 2021 •

edited

Loading

zannkukai Jan 15, 2021

zannkukai Jan 15, 2021

jma Jan 18, 2021

zannkukai Jan 15, 2021

zannkukai Jan 15, 2021

zannkukai Jan 15, 2021

zannkukai Jan 15, 2021

zannkukai Jan 15, 2021

zannkukai Jan 15, 2021

zannkukai Jan 15, 2021

sebdeleze commented Jan 19, 2021

sebdeleze commented Jan 19, 2021

sebdeleze commented Jan 19, 2021 •

edited

Loading

jma commented Jan 20, 2021

iGormilhit left a comment

sebdeleze Jan 29, 2021

		const i = this.getIndex() + 1;
		return this.field.parent.templateOptions.add(this.getIndex() + 1);

editor: fix issues #338

editor: fix issues #338

Conversation

jma commented Jan 7, 2021 • edited Loading

Why are you opening this PR?

How to test?

Code review check list

sebdeleze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebdeleze commented Jan 12, 2021

zannkukai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebdeleze Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebdeleze commented Jan 19, 2021

sebdeleze commented Jan 19, 2021

sebdeleze commented Jan 19, 2021 • edited Loading

jma commented Jan 20, 2021

iGormilhit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jma commented Jan 7, 2021 •

edited

Loading

sebdeleze Jan 19, 2021 •

edited

Loading

sebdeleze commented Jan 19, 2021 •

edited

Loading