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.

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

Annotation Modeler UI Text Review #2870

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

Conversation

lfindlaysap
Copy link
Contributor

Reviewed the Annotation Modeler UI texts in this repo.

@lfindlaysap lfindlaysap added the documentation Improvements or additions to documentation label Feb 5, 2025
@lfindlaysap lfindlaysap requested a review from JannaLisa February 5, 2025 09:50
@lfindlaysap lfindlaysap self-assigned this Feb 5, 2025
@lfindlaysap lfindlaysap requested a review from a team as a code owner February 5, 2025 09:50
Copy link

cla-assistant bot commented Feb 5, 2025

CLA assistant check
All committers have signed the CLA.

@JannaLisa
Copy link

one general comment: we were told for FE by our language editor some time ago that it's more natural to say/write "The {{name}} property is not allowed here" as opposed to "The property {{name}} is not allowed here."
Until then, I also always did it the way you have it here. Please just give it some thought, you could maybe also do a spotcheck what the common way in the FT docu is, and if it were really worth the effort of changing it. If you consistently already do it like you do it here, it's probably easiest to just stick with it. Honestly, it never bothered me, I wouldn't have changed if our language editor hadn't pointed it out.
Just a random example from FE:
According to the defined value in the chartType property of the UI.Chart annotation the corresponding MicroChart control is rendered

Copy link

@JannaLisa JannaLisa left a comment

Choose a reason for hiding this comment

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

A few small comments from me.

Copy link

changeset-bot bot commented Feb 7, 2025

⚠️ No Changeset found

Latest commit: 390f394

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@lfindlaysap
Copy link
Contributor Author

one general comment: we were told for FE by our language editor some time ago that it's more natural to say/write "The {{name}} property is not allowed here" as opposed to "The property {{name}} is not allowed here." Until then, I also always did it the way you have it here. Please just give it some thought, you could maybe also do a spotcheck what the common way in the FT docu is, and if it were really worth the effort of changing it. If you consistently already do it like you do it here, it's probably easiest to just stick with it. Honestly, it never bothered me, I wouldn't have changed if our language editor hadn't pointed it out. Just a random example from FE: According to the defined value in the chartType property of the UI.Chart annotation the corresponding MicroChart control is rendered

Sure. I prefer that suggestion and agree it's more natural. I saw the original many times across the docu so assumed that was the SAP convention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants