-
Notifications
You must be signed in to change notification settings - Fork 370
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: [ASM] Adding link to customer list for navigating to customer360 #18816
base: develop
Are you sure you want to change the base?
Conversation
…into feature/CXSPA-6858
4 flaky tests on run #43742 ↗︎
Details:
regression/asm/asm.emulation.core-e2e.cy.ts • 1 flaky test • B2C
ssr/pages.core-e2e.cy.ts • 3 flaky tests • SSR
Review all test suite changes for PR #18816 ↗︎ |
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.
just two same small comments
); | ||
|
||
this.subscription.add( | ||
this.launchDialogService?.dialogClose |
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.
? is redundant.
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.
copy from below code, I guess yes, we can remove the ?
createCustomer(): void {
this.launchDialogService?.openDialogAndSubscribe(
LAUNCH_CALLER.ASM_CREATE_CUSTOMER_FORM,
this.addNewCustomerLink
);
}
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.
yes, we can also remove it.
I guess that we fristly add 'launchDialogService' to custructor with a breakchange, then to haddle the break change, we had to difine it as
constructor(
............
protected launchDialogService**?**: LaunchDialogService
) {}
and in the major version, we don't remove all redundant code.
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.
Yes, I guess so
See https://jira.tools.sap/browse/CXSPA-6858