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

[UIST] Add new object type UIST #497

Merged
merged 27 commits into from
Mar 21, 2023
Merged

[UIST] Add new object type UIST #497

merged 27 commits into from
Mar 21, 2023

Conversation

ViktoriaFreidel
Copy link
Contributor

No description provided.

@cla-assistant
Copy link

cla-assistant bot commented Feb 27, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cla-assistant
Copy link

cla-assistant bot commented Feb 27, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@schneidermic0 schneidermic0 mentioned this pull request Feb 28, 2023
Closed
schneidermic0
schneidermic0 requested changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Thanks, @ViktoriaFreidel for your pull request.

I reviewed your proposal and added some questions and comments.

file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
schneidermic0
schneidermic0 reviewed Mar 8, 2023
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

I have just added some proposals to get rid of the "multipleOf"-issue we see in this PR

file-formats/uist/examples/z_aff_example_uist.uist.json Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/uist-v1.json Outdated Show resolved Hide resolved
@schneidermic0 schneidermic0 mentioned this pull request Mar 13, 2023
Closed
schneidermic0
schneidermic0 requested changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

I tried to summarise what we discussed earlier today

file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
schneidermic0
schneidermic0 requested changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Thank you for making so many improvements. I have added some small question.

file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/zif_aff_types_v1.intf.abap Show resolved Hide resolved
schneidermic0
schneidermic0 requested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I have found just three minor issue. I will ask my team members to double check it, too.

file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/uist/examples/z_aff_example_uist.uist.json Outdated Show resolved Hide resolved
file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated Show resolved Hide resolved
@schneidermic0
Copy link
Contributor

@ViktoriaFreidel Did you also sign the contributor license agreement (CLA)?

wurzka
wurzka reviewed Mar 16, 2023
View reviewed changes
Copy link
Contributor

@wurzka wurzka left a comment

Choose a reason for hiding this comment

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

I found some issues about the description. Could you please double check them, @ViktoriaFreidel ?
In contrast to the title, they shall not be in title case except when it is a common SAP term (like SAP Fiori).

file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated
"! $maximum: 999.999
sort_priority TYPE p LENGTH 7 DECIMALS 3,
"! <p class="shorttext">Base Space Template Name</p>
"! Base Space Template Name
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about SAP Terms here, but I think at least 'name' should be lower case, no?

Suggested change
"! Base Space Template Name
"! Base Space Template name

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could change it to "Name of the Base Space Template"? also not sure about the writing of "Base Space Template"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We change it to "Name of the base space template" - as description .
... Write the description in sentence case .... - from documentation.

file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated
"! Properties
BEGIN OF ty_general_information,
"! <p class="shorttext">Title</p>
"! Space Template Title
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

Suggested change
"! Space Template Title
"! Space Template title

file-formats/uist/type/zif_aff_uist_v1.intf.abap Outdated
"! SAP Fiori Page Template
BEGIN OF ty_page,
"! <p class="shorttext">Name</p>
"! Page Template Name
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"! Page Template Name
"! Page Template name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we change all description. Thanks

file-formats/uist/type/zif_aff_uist_v1.intf.abap Show resolved Hide resolved
schneidermic0
schneidermic0 approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 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 for me, now.

We have to ignore the validation error for now. This is in progress to be fixed. See #498

@ViktoriaFreidel
Copy link
Contributor Author

@schneidermic0 : To the Question
"Did you also sign the contributor license agreement (CLA)?"

Answer: No, not yet. The Link to CLA Page is not working at the moment.

@DominikvonSchoenebeck
Copy link
Contributor

Viktoria is out of office for a longer time.
Therefore I will act as substitute.

@DominikvonSchoenebeck
Copy link
Contributor

I could sign the CLA successfully.

@schneidermic0
Copy link
Contributor

@DominikvonSchoenebeck The change is fine, now. You can consider it as reviewed

If you don't mind we won't merge this PR until the issue with JSON schema validation is fixed (see #498). The team is working on a fix and plans to provide it somewhen next week.

@DominikvonSchoenebeck
Copy link
Contributor

@schneidermic0 We are fine with that. Thanks for the review.

@schneidermic0
Copy link
Contributor

#498 is fixed. Thanks, @burcuka.

@ViktoriaFreidel @DominikvonSchoenebeck I'll merge this PR. Welcome as contributor 👍

@schneidermic0 schneidermic0 merged commit 573fc5a into SAP:main Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wurzka wurzka wurzka left review comments

@huber-nicolas huber-nicolas huber-nicolas left review comments

@burcuka burcuka burcuka left review comments

@larshp larshp larshp left review comments

@schneidermic0 schneidermic0 schneidermic0 approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@ViktoriaFreidel @schneidermic0 @DominikvonSchoenebeck @larshp @burcuka @huber-nicolas @wurzka