-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix: static webapp path #2931
base: main
Are you sure you want to change the base?
fix: static webapp path #2931
Conversation
🦋 Changeset detectedLatest commit: 73c579d The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Changes for @sap-ux/mockserver-config-writer
and @sap-ux/odata-service-writer
looks good from my side.
Changeset corresponds to the changes. Approved.
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.
Code changes are clear.
assumed covered by tests
changeset ✅
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.
- Focus on
create
, reusing refactored method lg - test snapshots show improved webapp path
- changeset provided
- Did not test manually
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 @heimwege, update to use getWebappPath()
looks good. I commented some cosmetic proposals.
packages/adp-tooling/src/prompts/add-annotations-to-odata/index.ts
Outdated
Show resolved
Hide resolved
packages/adp-tooling/src/prompts/add-annotations-to-odata/index.ts
Outdated
Show resolved
Hide resolved
defc2ba
@mmilko01 While I was implementing the changes suggested by @Klaus-Keller I also refactored how the ui5 yaml config is being read in adp-tooling. project-access has a method for that as well ( open-ux-tools/packages/adp-tooling/src/base/helper.ts Lines 63 to 80 in defc2ba
At least the tests are still green 🤞🏻 |
…tic-webapp-path # Conflicts: # packages/odata-service-writer/src/data/annotations.ts
@heimwege thanks for the heads-up. This refactoring looks good. |
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.
Reapproving. Nice cleanup, thanks @heimwege
|
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.
re-approving
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 @heimwege!
- all my comments have been addressed
- changeset exists
- code coverage is great
- did a visual review, no manual test which I strongly suggest should be done for adaptation editor
Approved from my side.
#2589