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.

Sign up for GitHub

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

Jump to bottom

feat: Use tabs instead of radio group for pickup options #19406

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

feat: Use tabs instead of radio group for pickup options #19406

wants to merge 2 commits into from

Conversation

sdrozdsap
Copy link
Contributor

@sdrozdsap sdrozdsap requested a review from a team as a code owner October 16, 2024 12:02
@github-actions github-actions bot marked this pull request as draft October 16, 2024 12:02
@sdrozdsap sdrozdsap mentioned this pull request Oct 16, 2024
Closed
sdrozdsap
sdrozdsap commented Oct 16, 2024
View reviewed changes
...re-libs/pickup-in-store/components/presentational/pickup-options/pickup-options.component.ts
@@ -89,4 +142,46 @@ export class PickupOptionsComponent implements OnChanges {
// Return false to stop `onPickupOptionChange` being called after this
return false;
}

protected initializeTabs() {
this.tabs = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zeyber here I've tried to use the tabs$ input property here, however it didn't work for me. We can catch up on this issue on slack

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see there is a lot going on with this component with pre-existing change detection calls and emits. I don't think the TabComponent can really be designed to account for it. So, the way you update the tabs based on this component's logic is ok :)

@sdrozdsap sdrozdsap mentioned this pull request Oct 17, 2024
Closed
@sdrozdsap sdrozdsap marked this pull request as ready for review October 21, 2024 09:55
@github-actions GitHub Actions
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft October 21, 2024 12:32
@sdrozdsap sdrozdsap marked this pull request as ready for review October 21, 2024 12:32
@github-actions GitHub Actions
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@cypress Cypress
Copy link

cypress bot commented Oct 21, 2024 •
edited
Loading

spartacus    Run #45453

Run Properties:  status check failed Failed #45453  •  git commit 42f8307fc9 ℹ️: Merge 84683dab75f1fe99f090ba07c2b2685a70558fd7 into f797ca65c9892b124dd5ee29c729...
Project spartacus
Branch Review feature/CXSPA-7972
Run status status check failed Failed #45453
Run duration 07m 38s
Commit git commit 42f8307fc9 ℹ️: Merge 84683dab75f1fe99f090ba07c2b2685a70558fd7 into f797ca65c9892b124dd5ee29c729...
Committer sdrozdsap
View all properties for this run ↗︎
Test results
Tests that failed  Failures 2
Tests that were flaky  Flaky 6
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 123
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/regression/asm/asm.deeplink.core-e2e.cy.ts • 2 failed tests • B2C

View Output Video

Test Artifacts
Assisted Service Module > Customer Support Agent - Emulation&deeplink > should emulate customer and navigate to saved cart with deeplink before agent login (CXSPA-3263) Test Replay Screenshots Video
Assisted Service Module > Customer Support Agent - Emulation&deeplink > should emulate customer and navigate to saved cart with deeplink after agent login (CXSPA-3263) Test Replay Screenshots Video
Flakiness  regression/checkout/checkout-flow.core-e2e.cy.ts • 1 flaky test • B2C

View Output Video

Test Artifacts
Checkout flow > Mobile > should checkout with a registered user Test Replay Screenshots Video
Flakiness  regression/asm/asm.emulation.core-e2e.cy.ts • 1 flaky test • B2C

View Output Video

Test Artifacts
Assisted Service Module > Customer Support Agent - Emulation > should checkout as customer (CXSPA-7026) Test Replay Screenshots Video
Flakiness  regression/variants/apparel-checkout-as-guest.core-e2e.cy.ts • 1 flaky test • B2C

View Output Video

Test Artifacts
Apparel - checkout as guest > Desktop > should perform checkout as guest, create an account and verify guest data Test Replay Screenshots Video
Flakiness  ssr/pages.core-e2e.cy.ts • 3 flaky tests • SSR

View Output Video

Test Artifacts
SSR > should render homepage Test Replay Screenshots Video
SSR > should render PLP Test Replay Screenshots Video
SSR > should render PDP Test Replay Screenshots Video

@github-actions github-actions bot marked this pull request as draft October 21, 2024 13:29
@sdrozdsap sdrozdsap marked this pull request as ready for review October 21, 2024 13:31
@github-actions github-actions bot marked this pull request as draft October 21, 2024 18:37
@sdrozdsap
Copy link
Contributor Author

Cherry-picked commit from #19421 to fix the unit test failures

@sdrozdsap sdrozdsap marked this pull request as ready for review October 21, 2024 18:37
@github-actions github-actions bot marked this pull request as draft October 22, 2024 06:48
Zeyber
Zeyber requested changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@Zeyber Zeyber left a comment

Choose a reason for hiding this comment

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

I am happy to see that we will be able to clean up a lot of the complexity in future :)

Only minor comments from me. But lets wait to merge this since the TabComponent logic overlaps.

...re-libs/pickup-in-store/components/presentational/pickup-options/pickup-options.component.ts Outdated Show resolved Hide resolved
...re-libs/pickup-in-store/components/presentational/pickup-options/pickup-options.component.ts Outdated Show resolved Hide resolved
...re-libs/pickup-in-store/components/presentational/pickup-options/pickup-options.component.ts
@@ -89,4 +142,46 @@ export class PickupOptionsComponent implements OnChanges {
// Return false to stop `onPickupOptionChange` being called after this
return false;
}

protected initializeTabs() {
this.tabs = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see there is a lot going on with this component with pre-existing change detection calls and emits. I don't think the TabComponent can really be designed to account for it. So, the way you update the tabs based on this component's logic is ok :)

@Zeyber Zeyber marked this pull request as ready for review October 22, 2024 07:35
@github-actions GitHub Actions
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft October 23, 2024 11:20
@sdrozdsap sdrozdsap marked this pull request as ready for review October 23, 2024 12:01
@github-actions GitHub Actions
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zeyber Zeyber Zeyber requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sdrozdsap @Zeyber