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

Fix loading spinner issue on variant change #656

Merged
merged 4 commits into from
Sep 23, 2021
Merged

Fix loading spinner issue on variant change #656

merged 4 commits into from
Sep 23, 2021

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Sep 20, 2021

Why are these changes introduced?

Fixes #636

What approach did you take?

I moved the spinner outside of the button and added it as the next sibling element of the atc. This way the element isn't removed on variant changed (it was before as we update the textContent of the atc button).

Also added some product/feat. product specific styling to place the spinner relative to the button form.

Other considerations

Some options I considered:

  • Using the svg file as a pseudo element. But I couldn't animate the path properly to match the animation like in the rest of the theme.
  • I could move the spinner outside of the product-form__buttons element and position it absolutely, relative to the form. I will give this a try and report back. Edit: looks good this way.
  • Use JS to re insert the spinner element on variant change. But more JS isn't ideal.

Demo links

Checklist

@ludoboludo ludoboludo marked this pull request as ready for review September 20, 2021 18:15
@LucasLacerdaUX LucasLacerdaUX self-requested a review September 22, 2021 14:23
@KaichenWang KaichenWang self-requested a review September 22, 2021 15:12
Copy link
Contributor

@KaichenWang KaichenWang left a comment

Choose a reason for hiding this comment

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

This works fine, but maybe another approach would be to wrap the button label in a <span> and replace the textContent of only the label instead of the whole button?

I feel like toggleAddButton() is making an incorrect assumption that the add-to-cart button can only contain text and no children, so perhaps this root cause should be addressed.

@ludoboludo
Copy link
Contributor Author

Good point @KaichenWang 🤔 I've changed the approach following your suggestion. Let me know what you think :)

Copy link
Contributor

@KaichenWang KaichenWang left a comment

Choose a reason for hiding this comment

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

Works well

Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX left a comment

Choose a reason for hiding this comment

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

Works as expected! :dancing-razz:

The <span> idea was really nice and didn't break the layout :D

@ludoboludo ludoboludo merged commit 79f1014 into main Sep 23, 2021
@ludoboludo ludoboludo deleted the atc-issue branch September 23, 2021 13:49
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add to Cart Button Not Adding to Cart on Second Dimension Variants
3 participants