-
Notifications
You must be signed in to change notification settings - Fork 61
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
[SMBC] update 2411 #672
[SMBC] update 2411 #672
Conversation
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.
@sepp4me Thanks for sharing the update.
Sorry for the late review. I didn't manage to look into it earlier.
We need to discuss the change of the default value. From my point of view this is an incompatible change which could be covered by a new format version if needed.
Please also fix the ABAP lint issue
"! You can freeze the first columns of a table so that they always remain visible when scrolling the table horizontally | ||
"! $minimum 0 | ||
"! $maximum 30 | ||
"! $default '0' |
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.
Minor: I guess no default would be needed since 0 is automatically the default value for integers.
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.
ABAP lint and default value -> done
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 looks good to me. I am adding UX colleagues.
I see only a mismatch of a field name with its title. But due to length restrictions I also didn't come up with a better proposal
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.
Only one issue with the title, apart from that it looks good to me.
Co-authored-by: Katharina Wurz <[email protected]>
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, looks good to me from UX perspective
As changing the default from "auto" to "multi" is (semantically) a compatible change (see discussion), we can merge this change without increasing the format version. |
new attributes:
new default value for attribute selectionMode: Multi