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

[SMTG] Add new object type SMTG #633

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

[SMTG] Add new object type SMTG #633

wants to merge 15 commits into from

Conversation

OliverBeck01
Copy link

No description provided.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Jul 10, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Jul 10, 2024

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.

@GuilhermeSaraiva96
Copy link

missing an example as well

@albertmink albertmink added the new-object This is a new object type added to AFF label Jul 10, 2024
@huber-nicolas huber-nicolas self-assigned this Jul 11, 2024
huber-nicolas
huber-nicolas requested changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@huber-nicolas huber-nicolas 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 minor findings (see my comments). But I think that the AFF structure needs to be changed nearly completely. Maybe we can have a call and talk about it.

file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
@huber-nicolas
Copy link
Contributor

Please expect a delay for the review of this pull request. We have to clarify the language dependent fields of this object.

@huber-nicolas
Copy link
Contributor

huber-nicolas commented Aug 6, 2024 •
edited by Markus1812
Loading

Thank you for submitting your object type to the AFF Repository. After detailed discussion with @wurzka @schneidermic0 please go over my comments above and solve them. Please also solve the abaplint issues.

As we discussed in a previous meeting, please remove the ty_template_content, in order to put the language parts into separate files.

Markus1812
Markus1812 requested changes Aug 14, 2024
View reviewed changes
Copy link
Member

@Markus1812 Markus1812 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.

Thanks for updating the AFF. I've added some new comments with some optional suggestions that are up to you to decide.
If you have any questions regarding my comments, feel free to reach out.

file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
@OliverBeck01
Copy link
Author

Thank you for the feedback. I changed the requested lines and uploaded the updated documents.

Markus1812
Markus1812 reviewed Aug 28, 2024
View reviewed changes
Copy link
Member

@Markus1812 Markus1812 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.

Hi, thank you again for adapting our suggestions. One more question from our side:

Edit: Please also sign the Contributor License Agreement as described in #633 (comment) to make all automated checks happy.

file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
@Markus1812
Copy link
Member

Ok, thanks for the update. I'll add the ux-review-ready label so you can continue with the ux review when you are ready.

@Markus1812 Markus1812 added the ux-review ready AFF is ready for UX review label Oct 22, 2024
wurzka
wurzka requested changes Oct 23, 2024
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.

@OliverBeck01, could you please check the issue regarding name of structure vs title?

file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated
BEGIN OF ty_general_information,
"! <p class="shorttext">Template Description</p>
"! Long description to that email template
long_description TYPE c LENGTH 255,
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually recommend having title and name of the component in sync. Is it possible to rename to

Suggested change
long_description TYPE c LENGTH 255,
template_description TYPE c LENGTH 255,

? The other discrepancies between title and name are fine for me.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the description name to "template_description"

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a commit from you missing? I cannot see the change to "template_description". As soon as this is done, we can merge this PR 😊

Copy link
Author

@OliverBeck01 OliverBeck01 Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I waited for some more infos before uploading the latest version. I have now changed the layout of the main view so that two fields will no longer be visible (they are placed on the action popup instead). Will this require another UI review?
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuilhermeSaraiva96 GuilhermeSaraiva96 GuilhermeSaraiva96 left review comments

@wurzka wurzka wurzka requested changes

@huber-nicolas huber-nicolas huber-nicolas requested changes

@Markus1812 Markus1812 Markus1812 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@huber-nicolas huber-nicolas

Labels
new-object This is a new object type added to AFF ux-review ready AFF is ready for UX review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@OliverBeck01 @GuilhermeSaraiva96 @huber-nicolas @Markus1812 @wurzka @albertmink @schneidermic0