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

[ILMB] Add New Object ILMB #536

Merged
merged 21 commits into from
Sep 26, 2023
Merged

[ILMB] Add New Object ILMB #536

merged 21 commits into from
Sep 26, 2023

Conversation

srnawaz
Copy link
Contributor

@srnawaz srnawaz commented May 25, 2023

Adding ILMB Object to the GitHub Repository for UX Review

schneidermic0
schneidermic0 requested changes May 26, 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 @srnawaz. See my comments and questions below.

file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/README.md Outdated

File | Cardinality | Definition | Schema | Example
:--- | :--- | :--- | :--- | :---
`<name>.ilmb.json` | 1 | [`zif_aff_ilmb_v1.intf.abap`](./type/zif_aff_ilmb_v1.intf.abap) | [`ilmb-v1.json`](./ilmb-v1.json) | [`z_aff_example_ilmb.ilmb.json`](./examples/z_aff_example_ilmb.ilmb.json)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have either to add the example or remove the link. This fixes the broken link build error.

schneidermic0
schneidermic0 requested changes Jun 13, 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 @srnawaz for the update. Please see my comments below

file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
wurzka
wurzka requested changes Jun 13, 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.

Just two issues with 'Details' in the title

file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
@schneidermic0
Copy link
Contributor

Sorry, I haven't manage to look into the updated version, yet. I plan to look into it next week.

schneidermic0
schneidermic0 approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have deleted the link to the missing example for the time being.

Looks good to me. It can be reviewed by @SAP/abap-file-formats-ux

@schneidermic0 schneidermic0 requested a review from a team June 19, 2023 10:58
wurzka
wurzka requested changes Aug 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.

We managed to perform UX Review. See my comments

file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated
"! <p class="shorttext">Condition Field Details</p>
"! Condition field details
BEGIN OF ty_cond_field,
"! <p class="shorttext">Name</p>
Copy link
Contributor

@wurzka wurzka Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To have it in sync with time reference table:

Suggested change
"! <p class="shorttext">Name</p>
"! <p class="shorttext">Condition Field</p>

file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
@schneidermic0
Copy link
Contributor

@srnawaz When do you plan to update this pull request with the changes requested by @wurzka?

@cla-assistant
Copy link

cla-assistant bot commented Sep 25, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cla-assistant
Copy link

cla-assistant bot commented Sep 25, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ schneidermic0
✅ srnawaz
✅ wurzka
❌ KARSUV
You have signed the CLA already but the status is still pending? Let us recheck it.

wurzka
wurzka requested changes Sep 25, 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.

There are two issues with the description: The description should be in sentence case and not in title case

file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ilmb/type/zif_aff_ilmb_v1.intf.abap Outdated Show resolved Hide resolved
wurzka
wurzka approved these changes Sep 25, 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.

Looks good to me, now 👍

@wurzka
Copy link
Contributor

wurzka commented Sep 25, 2023

I created a issue for not forgetting the example: #559

@schneidermic0 schneidermic0 merged commit 2cd0a39 into SAP:main Sep 26, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schneidermic0 schneidermic0 schneidermic0 approved these changes

@wurzka wurzka wurzka 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.
4 participants
@srnawaz @schneidermic0 @wurzka @KARSUV