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

Feat/make rta url params optional #2431

Merged
merged 19 commits into from
Oct 22, 2024
Merged

Conversation

heimwege
Copy link
Contributor

@heimwege heimwege commented Oct 1, 2024

This PR makes the usage of url params for the rta editor endpoints optional. In case they are missing a re-route will happen and the parameters will be added 🥳

Copy link

changeset-bot bot commented Oct 1, 2024

🦋 Changeset detected

Latest commit: 69731d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@sap-ux/preview-middleware Patch
@sap-ux/create Patch

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

@heimwege
Copy link
Contributor Author

heimwege commented Oct 1, 2024

@ytpo-lyne this PR should help us for https://github.com/SAP/open-ux-tools/tree/feat/create-variantsConfig because the url parameter for the rta editor endpoints are no longer needed with this PR.
@tobiasqueck or do we need to keep them because of some downward compatibility? But if this PR is merged first we should hopefully be fine 🤔

@heimwege
Copy link
Contributor Author

heimwege commented Oct 1, 2024

@tobiasqueck I think we do not need to insist on this logic working with the connect API used by the karma test runner as rta editor is not subject to karma tests. Would you agree?

@heimwege heimwege marked this pull request as ready for review October 1, 2024 16:16
@heimwege heimwege requested a review from a team as a code owner October 1, 2024 16:16
Copy link
Contributor

@tobiasqueck tobiasqueck left a comment

Choose a reason for hiding this comment

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

See inline comment - the redirects for tests with developerMode=true are not expected.

packages/preview-middleware/test/unit/base/flp.test.ts Outdated Show resolved Hide resolved
tobiasqueck
tobiasqueck previously approved these changes Oct 22, 2024
Copy link
Contributor

@tobiasqueck tobiasqueck left a comment

Choose a reason for hiding this comment

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

  • changeset ok
  • code change is simple BUT very useful since it allows us to simplify the generated call scripts
  • didn't test locally but reviewing the jest tests gives me enough confidence

tobiasqueck
tobiasqueck previously approved these changes Oct 22, 2024
Copy link
Contributor

@ytpo-lyne ytpo-lyne left a comment

Choose a reason for hiding this comment

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

  • changeset present
  • code changes are straight forward
  • code coverage is excellent
  • tested locally, no issues found

Copy link

sonarcloud bot commented Oct 22, 2024

@heimwege heimwege merged commit 3e9cab4 into main Oct 22, 2024
13 checks passed
@heimwege heimwege deleted the feat/make-rta-url-params-optional branch October 22, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants