Use a theme function for rendering "there is no shipping rate"

Great! We should combine this with 🐛 Improve "no shipping rates" message. Needs review in one of both issues, I think.

I think instead of the <p> we should use a <div> with a dedicated and a general (warning / error) class.

Furthermore, we should decide for one of both issues to also discuss the other points mentioned over there in 🐛 Improve "no shipping rates" message. Needs review .

This is the older issue, but due to the discussion and other points over there, I'd suggest to move the solution from here over and close this issue!

Please don't forget crediting @renrhaf!

From the other issue:

Thanks @Grevil, I agree that this is currently a bit confusing. Would be even better to also adjust the Drupal form message (empty required field) if no shipping method matches.

Maybe we should even allow defining a configurable fallback message for that case as a setting, so that shop owners can explain the potential reasons in this case instead of a hard-coded message?

Let's wait for Centarro feedback on that.

Regarding the class: I think it would make sense to add a context class like you did, and also add a Drupal error or warning message class to show the message appropriately by default.

I'm unsure if such a class exists, or if that means we'd have to use something like the message's markup?

@jsacksick what are your thoughts here?

I'd rather fix the issue here.

grevil → changed the visibility of the branch 8.x-2.x to hidden.

Done. Please review!

Thanks, @Anybody! I used a bad example to implement the attributes. LGTM now!

<div class="commerce-shipping-no-shipping-rates" data-drupal-selector="edit-shipping-information-shipments-0-shipping-method-0">
  Es sind keine Versandkosten für diese Adresse verfügbar.
</div>

@grevil thanks! Could you provide a before / after screenshot perhaps?

Code-wise I think this is helpful and a lot more flexible than before! RTBC from my side. For further discussion:

It would be great to add a warning class, but I think also Olivero has no warning-styles outside of messages and I don't think it would be correct to add message markup here?

Another thing I thought about is, if it might be helpful for any case to pass the $shipment variable to the twig file?

Let's wait for maintainer feedback on this in general and these points.

Before:

After:

Added the shipment entity to the theme, might make sense to get the title of the shipment or something along those lines.

Nice and thank you for the before / after screenshots! RTBC

hm... I don't really see any difference between the before / after? Are we supposed to see any?

hm... I don't really see any difference between the before / after? Are we supposed to see any?

No not really! But it's easily overridable now. :)

Not really RTBC, there are phpcs failures, tests are failing too with D11, but I don't think the failures are related.

As of #19, thanks for the feedback. PHPCS also seem unrelated in large parts, but we'll check that...

Current changes don't contain anything that would violate the Drupal coding standards, the phpunit test seems very unrelated as well. I bet, if we rerun the pipeline on the current dev build, we get the same errors.

Yep, simply adding another hashtag in the .gitlab-ci.yml will result in the same pipeline failures (see MR !50). The pipeline in 8.x-2.x will have the same errors if rebuilt.

Setting back to RTBC, as the failures are unrelated.

Shouldn't we have a class name consistent with the theme function? (i.e. commerce-shipping-rates-empty?)

Sure! Adjusted accordingly.

Thanks, @jsacksick! 🎉

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