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.

Sign up for GitHub

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

Jump to bottom

Angular renderer for object arrays #1615

Merged
merged 4 commits into from
Aug 12, 2020
Merged

Angular renderer for object arrays #1615

merged 4 commits into from
Aug 12, 2020

Conversation

AlexandraBuzila
Copy link
Member

Add array layout renderer for objects.

eneufeld
eneufeld reviewed Jul 29, 2020
View reviewed changes
Copy link
Member

@eneufeld eneufeld left a comment

Choose a reason for hiding this comment

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

Could you check whether by extending JsonFormsArrayControl we can get rid of some methods?

packages/angular-material/src/layouts/array-layout.renderer.ts Outdated
`,
changeDetection: ChangeDetectionStrategy.OnPush
})
export class ArrayLayoutRenderer extends JsonFormsBaseRenderer<ControlElement>
Copy link
Member

Choose a reason for hiding this comment

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

why not extend the JsonFormsArrayControl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I changed the renderer so it extends JsonFormsArrayControl

packages/angular-material/src/layouts/array-layout.renderer.ts Outdated
mat-icon-button
[disabled]="readonly"
(click)="add()"
attr.aria-label="{{ 'Add to ' + this.label + 'button' }}"
Copy link
Member

Choose a reason for hiding this comment

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

we will need to find a localization solution

eneufeld
eneufeld reviewed Jul 29, 2020
View reviewed changes
packages/angular-material/src/layouts/array-layout.renderer.ts Outdated

getProps(index: number): OwnPropsOfRenderer {
const uischema =
(this.uischema.options?.detailUISchema as UISchemaElement) ??
Copy link
Member

Choose a reason for hiding this comment

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

we should unify this with the react renderer and use detail

Copy link
Member

Choose a reason for hiding this comment

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

or even better use the findschema like here:

jsonforms/packages/material/src/layouts/ExpandPanelRenderer.tsx

Line 101 in 6cf39ef

const foundUISchema = findUISchema(

eneufeld
eneufeld reviewed Jul 30, 2020
View reviewed changes
packages/angular-material/src/layouts/array-layout.renderer.ts Outdated

export const ArrayLayoutRendererTester: RankedTester = rankWith(
4,
isObjectArray
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we check for isObjectArrayWithNesting instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that tester too restrictive? If we don't have a nested array or don't provide a detail schema in the options, then the new renderer won't be used.

eneufeld
eneufeld reviewed Aug 6, 2020
View reviewed changes
packages/angular-material/src/layouts/array-layout.renderer.ts Outdated
uischema: UISchemaElement;
}[];

constructor(private jsonFormAngularService: JsonFormsAngularService) {
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to make this private as we can access it from the parent

Copy link
Member

Choose a reason for hiding this comment

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

If not, it should be accessible via the parent

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

eneufeld
eneufeld approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member

@eneufeld eneufeld left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you. I added a small possible improvement comment, it can be done as a follow up though.

@eneufeld
Copy link
Member

eneufeld commented Aug 6, 2020

Do you know why neither the buttons nor the toolbar is using the default styling colors of material?

@AlexandraBuzila
Copy link
Member Author

AlexandraBuzila commented Aug 7, 2020 •
edited
Loading

I did the small refactoring to the array renderer.

Regarding the styling:

  • the button style was not consistent with the other renderers, I fixed that
  • the toolbar styling was a little different because we were not using the Roboto font. I added it to the Angular Material examples, but I can revert this change if you don't agree with it.
    The toolbar should look now like in the Angular Material documentation: https://material.angular.io/components/toolbar/overview

@AlexandraBuzila
Copy link
Member Author

Ah, and the toolbar color is not set to 'primary'. I'll update my commit to use it.

@AlexandraBuzila
Fix styling for array layout renderer
06321ed 
- use correct button
- add Roboto font to Angular Examples
- use 'primary' color for toolbar
@coveralls
Copy link

coveralls commented Aug 7, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 88.78% when pulling 06321ed on AlexandraBuzila:array-renderer into a2254ef on eclipsesource:master.

sdirix
sdirix approved these changes Aug 12, 2020
View reviewed changes
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Works for me!

@sdirix sdirix merged commit 15348c0 into eclipsesource:master Aug 12, 2020
@sdirix sdirix added this to the 2.4.1 milestone Aug 12, 2020
@AlexandraBuzila AlexandraBuzila deleted the array-renderer branch August 12, 2020 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdirix sdirix sdirix approved these changes

@eneufeld eneufeld eneufeld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexandraBuzila @eneufeld @coveralls @sdirix