-
Notifications
You must be signed in to change notification settings - Fork 56
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
[UIAD] Add new object type UIAD #586
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aaronbruchsap,
thanks for contributing the UIAD to our repository. I went through your ABAP type and put some comments.
Don't be afraid of the number 😉 . Most of them are related to the title case. There are also some questions or suggestions for improvement we maybe want to discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for your changes @aaronbruchsap! I just have one more question.
However, before merging, I would like to also have a review and feedback from @schneidermic0. Therefore, I put him as reviewer.
Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
Check fails due to SchemaStore/schemastore#3620 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronbruchsap Thanks for all the updates.
Please find some questions and detailed suggestions below.
I am struggling a bit in making a proper proposal for the general structure. I don't see an option to provide a more flat structure. Depending on the tile types the list of tiles has still a deeply nested structure containing further arrays in array.
FYI: I think it is done.We commited new changed files and would like to use those for next UX-Review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and LGTM
we did the ux review together, so I overrole
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it looks good to me. I have just faced one question regarding default values for booleans.
Furthermore, following comment has not been answered, yet. This might lead to a change:
https://github.com/SAP/abap-file-formats/pull/586/files/89d45870b2fc419bbc31088d85ce266ef38c51e8..983a52c40896cb20f0041de140c213ddfbd99d39#r1521536216
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correction for default value and some titles is done. New files for UIAD are ready for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViktoriaFreidel Thank you very much for the update and removing the default.
Most probably the rename of fields make sense. I am wondering whether you can also adapt the field names. This will help users to find the correct field names in the JSON representation.
@SAP/abap-file-formats-ux Any concerns?
Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
AbapLint Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
abapLint Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sur if my approvement is needed, just confirm review changes
I just re-requested the review of @schneidermic0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
No description provided.