-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Including SAP as a provider #1153
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
base: master
Are you sure you want to change the base?
Conversation
* Adding SAP provider configuration * Adding resources of provider SAP * Adding badge of SAP provider * Adding SAP provider in the documentation * Adding SAP ERP S4HANACloud * Including additional SAP SaaS and ERP resources * Removing resources with wrong name * Updating BTP icons * Refreshed icons * Including new icons * Including sample python scripts with new icons * Updating icons for SAP Build Apps and Event Broker * Including event broker * Removing btp-diagrams-icons sample * Incorporating latest icons * Incorporate latest icons for AEM and SBPA * Latest integration icons * Syncing icons with the BTP solution diagrams structure * Including SAP cleaner --------- Co-authored-by: Antonio Maradiaga <[email protected]>
|
Hey @gabriel-tessier @ajmaradiaga |
|
@filipeaaoliveira @gabriel-tessier thanks for reviewing the PR. Unfortunately, a job failed because of a file that was incorrectly included in the repo. I removed tech-byte-diagram.py, which was included by mistake in the repository's root directory. This file caused a job run to fail: https://github.com/mingrammer/diagrams/actions/runs/15827106712/job/45501928915. It is not really needed hence why I just removed it in the last commit - 7be6f2f. |
|
@filipeaaoliveira @gabriel-tessier, I'm sorry this failed again. I ran the pre-commit hook locally and fixed the file causing the issue. This is another very minor fix. |
|
@ajmaradiaga There's a couple of icons like this one: There's other icons that are way too small like, they may not render well: And other that are too big like which make the package more heavy: For the 2 first pattern maybe it need to rework and can be done later but if you can run a script and resize the big pictures to maximum 256, as stated in the contribution file here: https://github.com/mingrammer/diagrams/blob/master/CONTRIBUTING.md You should be able to ask some AI to build a script to run in the SAP directory and resize any pictures that is bigger than 256. Maybe can ask about up-scaling the small one too. Thank you again for adding a new provider. |
gabriel-tessier
left a comment
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.
I checked 514 files and only found one big picture and the SAP logo that is too small.
If it's not too much work it should be fine to address this 2 changes so we can merge the PR.
The 2 other pattern I listed are fine and should not block, forget about my previous comment to build a script to detect ad resize big pictures actually it should be done on our side when building the package, ensuring that the icons are not too big.
@ajmaradiaga
Thank you again for this monumental work.
@filipeaaoliveira
Thank you for the first review.
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.
Does this icon really describe the service for the end user? I'm not a SAP use so I cannot judge but if there's not yet icon it can be a good place holder at least to promote the service and maybe get future contribution.
This comment is not intended to blocking the merge it's just a comment, I think it' ok to merge.
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.
This file is way too small and will not render correctly, there's other provider that have the same problem with small problem, I just pinpoint this if somebody notice and may provide a better icon.
Here also this comment is not blocking.
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.
This icon is too big and can be reduce to fit the size defined in the contribution guide.
Please resize the icon to match the contribution guide.
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.
I'm sure this one have a better version that match the minimum size of 256
|
Hi, any news ? I need Sap for my project. |
Adding SAP provider configuration
Adding resources of provider SAP
Adding badge of SAP provider
Adding SAP provider in the documentation
Adding SAP ERP S4HANACloud
Including additional SAP SaaS and ERP resources
Removing resources with wrong name
Updating BTP icons
Refreshed icons
Including new icons
Including sample python scripts with new icons
Updating icons for SAP Build Apps and Event Broker
Including event broker
Removing btp-diagrams-icons sample
Incorporating latest icons
Incorporate latest icons for AEM and SBPA
Latest integration icons
Syncing icons with the BTP solution diagrams structure
Including SAP cleaner