Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

Wim Leers’s picture

Issue tags: +Usability

Why optional? Why not enabled by default? This is a huge usability win.

bojanz’s picture

It was off by default in the D7 module, and my previous checkout research (looking at major sites) didn't show it to be common.
I'd be happy to re-investigate and flip that boolean, once we have the actual code for it :)

bojanz’s picture

Issue tags: +DevDaysSeville
finne’s picture

Assigned: Unassigned » finne
finne’s picture

Status: Active » Needs review
Lukas von Blarer’s picture

Status: Needs review » Needs work

The pull request doesn't apply anymore.

mglaman’s picture

Assigned: finne » mglaman

I'd like to postpone this until #2869818: Checkout progress block does not set current step appropriately when 'go back' link is used is merged. This provides a utility method for testing the currently highlighted step, and adds test coverage to the fact they even work.

mglaman’s picture

Lukas von Blarer’s picture

Does this only need a reroll or do we need to implement it diffrently?

mglaman’s picture

It needs a re-roll and tests. The issue I referenced in #9 provides test coverage for the block, which we were missing. So it'll be easier to work on this, now.

Lukas von Blarer’s picture

ok, i'll give it a try tomorrow.

Lukas von Blarer’s picture

Status: Needs work » Needs review

I re-rolled the pull request here and improved/fixed the tests:

https://github.com/drupalcommerce/commerce/pull/732

Lukas von Blarer’s picture

Status: Needs review » Needs work

The last submitted patch, 14: progress-links-2859834-14.patch, failed testing. View results

flocondetoile’s picture

Status: Needs work » Needs review
FileSize
9.52 KB

Patch #14 rerolled for commerce 2.5

Status: Needs review » Needs work

The last submitted patch, 16: progress-links-2859834-16.patch, failed testing. View results

flocondetoile’s picture

Status: Needs work » Needs review
FileSize
9.52 KB

Status: Needs review » Needs work

The last submitted patch, 18: progress-links-2859834-18.patch, failed testing. View results

flocondetoile’s picture

Status: Needs work » Needs review
FileSize
10.61 KB
mglaman’s picture

Assigned: mglaman » Unassigned
Getekid’s picture

Hello, I can confirm the patch is working as expected.

Would it make sense to expose the link to the steps array? This way one could use it in a custom template e.g. to assign classes to the link tag.

Thanks a lot for your work

heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
heddn’s picture

flocondetoile’s picture

Patch rerolled for commerce 2.14

Status: Needs review » Needs work

The last submitted patch, 25: 2859834-25.patch, failed testing. View results

flocondetoile’s picture

Status: Needs work » Needs review
FileSize
9.3 KB

Patch rerolled and updated with new behavior address book

jnrfred’s picture

Status: Needs review » Reviewed & tested by the community

Patch tested and works

flocondetoile’s picture

michiellucas’s picture

I created this block for my project but used a different approach.

  public function build() {
    $order = $this->routeMatch->getParameter('commerce_order');
    if (!$order) {
      // The block is being rendered outside of the checkout page.
      return [];
    }
    $render = parent::build();

    foreach ($render['#steps'] as $id => &$step) {
      /** @var \Drupal\commerce_order\Entity\OrderInterface $order */
      $step_id = $this->checkoutOrderManager->getCheckoutStepId($order, $step['id']);

      if ($step['id'] == $step_id) {
        $url = Url::fromRoute('commerce_checkout.form', [
          'commerce_order' => $order->id(),
          'step' => $step['id'],
        ]);

        $step['label'] = Link::fromTextAndUrl($step['label'], $url);
      }
    }

    return $render;
  }

You are checking on if ($index <= $current_step_index) { but you can use the same logic as CheckoutController::formPage

Sumi’s picture

Tested the patch and checkbox for enabling this functionality doesn’t do anything. Checkbox is unchecked by default but steps in the progress block are links. No matter if the checkbox is checked or not steps are links.

Sumi’s picture

Status: Reviewed & tested by the community » Needs work
andypost’s picture

andypost’s picture

There's no link in markup, means test needs more work

<div id="block-k9qfsols" class="block block-commerce-checkout block-commerce-checkout-progress">
  
      <h2>cJMu7F7I</h2>
    
      <ol class="checkout-progress clearfix">
        <li class="checkout-progress--step checkout-progress--step__current">Order information</li>
        <li class="checkout-progress--step checkout-progress--step__next">Review</li>
        <li class="checkout-progress--step checkout-progress--step__next">Complete</li>
  </ol>
andypost’s picture

mglaman’s picture

Status: Needs work » Needs review
Sumi’s picture

Status: Needs review » Needs work

Still checkbox "Display checkout progress breadcrumb as links" is not doing anything. It is unchecked by default but steps are links, it doesn't matter what is set on that checkbox.

mglaman’s picture

  1. +++ b/modules/checkout/src/Plugin/Block/CheckoutProgressBlock.php
    @@ -109,9 +110,20 @@ class CheckoutProgressBlock extends BlockBase implements ContainerFactoryPluginI
    +      // Create breadcrumb style links for active checkout steps.
    +      if ($index <= $current_step_index) {
    +        $label = Link::createFromRoute($step_definition['label'], 'commerce_checkout.form', [
    +          'commerce_order' => $order->id(),
    +          'step' => $step_id,
    +        ])->toString();
    +      }
    +      else {
    +        $label = $step_definition['label'];
    +      }
    
    +++ b/modules/checkout/src/Plugin/Commerce/CheckoutFlow/CheckoutFlowBase.php
    @@ -235,6 +235,7 @@ abstract class CheckoutFlowBase extends PluginBase implements CheckoutFlowInterf
    +      'display_checkout_progress_breadcrumb_links' => FALSE,
    

    This doesn't respect display_checkout_progress_breadcrumb_links

  2. +++ b/modules/checkout/templates/commerce-checkout-progress.html.twig
    @@ -14,6 +14,6 @@
    +      <li class="checkout-progress--step checkout-progress--step__{{ step.position }}">{{ step.label | raw }}</li>
    

    Why did this have to be changed to raw?

simgui8’s picture

# 35 works for me, but the checkbox ain't

simgui8’s picture

Here is a rerolled patch,
+
The raw filter has been removed
and I have added a display_checkout_progress_breadcrumb_links configuration verification.

Hope it address 1. and 2.

simgui8’s picture

Status: Needs work » Needs review
Sumi’s picture

Tested the patch and links are now respecting the backend configuration and are working as expected. There is only one thing and that is once user finishes the checkout and is on "Complete" step, previous steps are links but at that point you can't actually go back to "Review" or "Order information" steps so when you click on the links your page just reloads. It would probably be better that once you are on "Complete" step other steps are no longer links

mglaman’s picture

Status: Needs review » Needs work

Per #42, this needs work.

+++ b/modules/checkout/src/Plugin/Block/CheckoutProgressBlock.php
@@ -109,9 +110,23 @@ class CheckoutProgressBlock extends BlockBase implements ContainerFactoryPluginI
+      // Create breadcrumb style links for active checkout steps.
+      if (
+        $configuration['display_checkout_progress_breadcrumb_links'] &&
+        $index <= $current_step_index
+      ) {

This should check if the current step is the last step as well.

simgui8’s picture

This one check if current step is 'complete'

simgui8’s picture

Status: Needs work » Needs review
Sumi’s picture

Tested the patch and all works as expected.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/modules/checkout/src/Plugin/Block/CheckoutProgressBlock.php
@@ -112,6 +112,7 @@
+        $current_step_id !== 'complete' &&

💪Thanks!

Per #46 this is +1, will commit later today.

  • mglaman committed 643bb39 on 8.x-2.x authored by simgui8
    Issue #2859834 by flocondetoile, simgui8, andypost, Lukas von Blarer,...
mglaman’s picture

Status: Reviewed & tested by the community » Fixed

Committed 🥳! Thanks all.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.