Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Featured Product: Try using Button block for button #398

Merged
merged 4 commits into from
Feb 8, 2019

Conversation

ryelle
Copy link
Member

@ryelle ryelle commented Feb 4, 2019

See #368 – Use the Button block to render the featured product's call to action button. We set up the block to use a template with the only button block. There are a few gotchas by using the template, though.

  • The template is set once when the block loads, and does not reset if the product is changed - so if you pick Product A, the button sets up with the link to Product A; if you then change it to Product B, the link URL stays Product A. Any customized button text also sticks (but that's expected, IMO).
  • There's no way to pass settings into the block, so we can't disable editing of the link
  • We also can't disable the alignment options, so in this PR you need to align the text inside the block and then the button itself separately.

Bonuses we get from using the Button block:

  • Text & background colors, along with the contrast checker
  • The different Button styles are supported
  • The toolbar for the button text styles (bold, italic, etc) is only shown at the button level

Screenshots

It looks the same when inserted by default:

default

Since it's the button block, we can also use the different button styles:

styled

How to test the changes in this Pull Request:

  1. Add a Featured Product block
  2. Click around the button
  3. Expect: Should work like a button 🙂

Given the issues above (and the fact that some were explicitly called out as "don't do this" in #368) I'm looking more for design feedback than dev feedback ATM.

@ryelle ryelle added this to the 2.0 + Core Merge milestone Feb 4, 2019
@ryelle ryelle self-assigned this Feb 4, 2019
@ryelle ryelle requested review from jameskoster and a team February 4, 2019 22:08
Copy link
Member

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

Ok, the alignment "issue" isn't a big deal after all - happy with that. I did notice one small quirk with the behaviour though:

If you justify the block text right, then align the button left, the appearance isn't correct when the button block is highlighted. It then also becomes difficult to hover the button block with that alignment configuration - you have to click on the button directly.

gif

The bigger problem is the button URL not updating when switching products though. If it's not possible to do this then we'll have to remove the product switching feature. I'm sad that will mean lost work but I think button style editing is more important than product switching. It's easy to remove the block and add it again.

@ryelle ryelle force-pushed the add/featured-product-button branch from 7d80806 to 0f7fe1a Compare February 5, 2019 17:35
@ryelle
Copy link
Member Author

ryelle commented Feb 5, 2019

@jameskoster I've fixed the alignment issue. I was also thinking the solution to the URL thing would be to remove the product switching feature, so if you're OK with that, I'll go ahead with it (but in another PR, we can merge this one first).

@jameskoster
Copy link
Member

jameskoster commented Feb 5, 2019

Yep, works for me!

Edit: Actually, the display is fixed, but the hover effect is missing. If the button is aligned left or right you have to click on the actual button to focus the block.

Edit 2: That seems to be a core issue when buttons are nested inside other blocks. I'll open a core issue.

@ryelle
Copy link
Member Author

ryelle commented Feb 7, 2019

@jameskoster If this PR is OK design-wise, can you dismiss or update your review?

@ryelle ryelle force-pushed the add/featured-product-button branch from 0f7fe1a to 8c0e6dd Compare February 7, 2019 16:30
Copy link
Contributor

@coderkevin coderkevin left a comment

Choose a reason for hiding this comment

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

Code LGTM. 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants