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

issue #2859834 implementation of links in checkout progress block #686

Conversation

finnef
Copy link
Contributor

@finnef finnef commented Mar 22, 2017

No description provided.

@finnef
Copy link
Contributor Author

finnef commented Mar 22, 2017

@bojanz
Copy link
Contributor

bojanz commented Mar 22, 2017

We also need a setting for enabling this, like the D7 module had.

$label = Link::createFromRoute($step_definition['label'], 'commerce_checkout.form', [
'commerce_order' => $order->id(),
'step' => $step_id,
])->toString();

Choose a reason for hiding this comment

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

maybe ->to Renderable() will be better here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should always be renderable, unless that does fail

$label = Link::createFromRoute($step_definition['label'], 'commerce_checkout.form', [
'commerce_order' => $order->id(),
'step' => $step_id,
])->toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should always be renderable, unless that does fail

@@ -14,6 +14,6 @@
#}
<ol class="checkout-progress clearfix">
{% for step in steps %}
<li class="checkout-progress--step checkout-progress--step__{{ step.position }}">{{ step.label }}</li>
<li class="checkout-progress--step checkout-progress--step__{{ step.position }}">{{ step.label | raw }}</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove raw toRenderable should be fine

@finnef
Copy link
Contributor Author

finnef commented Mar 22, 2017

changed to Renderable() and removed | raw.
Also added config checkbox

Copy link
Contributor

@mglaman mglaman left a comment

Choose a reason for hiding this comment

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

    1)
    Drupal\Tests\commerce_checkout\Kernel\ChainCheckoutFlowResolverTest::testCheckoutFlowResolution
    Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for
    commerce_checkout.commerce_checkout_flow.default with the following errors:
    commerce_checkout.commerce_checkout_flow.default:configuration.display_checkout_progress_breadcrumb_links
    missing schema

the new option needs to be added to the module's schema.

can we also update a test where it clicks a link and goes back and continues?

@finnef
Copy link
Contributor Author

finnef commented Mar 24, 2017

Added tests and schema.

Copy link
Contributor

@mglaman mglaman left a comment

Choose a reason for hiding this comment

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

Almost thumbs up here. However our tests do not prove this works or document the interaction.

@@ -155,13 +166,14 @@ public function testRegisterOrderCheckout() {
'login[register][password][pass2]' => 'pass',
], 'Create account and continue');
$this->assertSession()->pageTextContains('Billing information');
// Check breadcrumbs are not links. (the default setting)
$this->assertSession()->elementNotExists('css', '.block-commerce-checkout-progress li.checkout-progress--step > a');
Copy link
Contributor

Choose a reason for hiding this comment

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

So we assert it's not a link, however we do not prove the feature works in any test

Copy link
Contributor Author

@finnef finnef Mar 24, 2017

Choose a reason for hiding this comment

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

The test to use the link to move back and fwd through the checkout steps is in testGuestOrderCheckout. The test for no link (by default) is in testRegisterOrderCheckout.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh duh, wow i totally missed that. All good then!

@bojanz
Copy link
Contributor

bojanz commented Sep 6, 2017

Continued (rebased) in #732.

@bojanz bojanz closed this Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants