cms-theme-boilerplate icon indicating copy to clipboard operation
cms-theme-boilerplate copied to clipboard

Add CTA functionality to button

Open TanyaScales opened this issue 3 years ago • 5 comments

  • Adding CTA field and choice between button and CTA to the module
  • Adjusting visibility of style fields when CTA is selected so they are hidden

Types of change

  • [ ] Bug fix (change which fixes an issue)
  • [x] Enhancement (change which improves upon an existing feature)
  • [ ] New feature (change which adds new functionality)

Description

This PR adds CTA functionality to the button module in the theme by:

  • Adding a choice field to choose between a button and CTA
  • Adding the if statement for the swap between markup
  • Setting visibility on the style fields so that they don't show up if CTA is chosen
  • Additionally, there are some markup changes:
    • encloses the href, target, and rel statements inside a macro and cleaning up floating href attribute when link is not set
    • ~Also adds further checks for telephone numbers, relative links, etc. for the href value.~ This wasn't necessary based on recent updated to the validation in the UI
    • Lastly, I've adjusted the rel attribute markup as well to remove the floating rel present when neither option is toggled
  • Adding hover style field options and :hover and :active declarations to the scoped CSS of the module

Relevant links

DM Previewer

Checklist

TanyaScales avatar Sep 09 '21 18:09 TanyaScales

I think previous iterations of the boilerplate were kept pretty simple and didn't include any hover styles for buttons. I definitely agree that we should have some hover options - do we think it makes sense to also add some in theme settings so there is parity with this module update? I think current theme settings also lacks hover controls. Happy to help if it makes sense to put up a PR separate from this one for that task.

jasonnrosa avatar Sep 13 '21 15:09 jasonnrosa

I think previous iterations of the boilerplate were kept pretty simple and didn't include any hover styles for buttons. I definitely agree that we should have some hover options - do we think it makes sense to also add some in theme settings so there is parity with this module update? I think current theme settings also lacks hover controls. Happy to help if it makes sense to put up a PR separate from this one for that task.

I would think that, yes, we should update to include hover in both places if we are going to include it here. I guess you bring up the larger question -- are we then saying that including hover state controls for marketers is best practice? Because that is what Boilerplate is supposed to be guiding. I can definitely see both sides to the argument where hover state controls might be considered a choice the developer makes based on their needs -- and maybe not necessarily always the best practice.

TanyaScales avatar Sep 13 '21 19:09 TanyaScales

Yeah the best practice thing is a good point. I was mainly thinking specifically about button but looking at hover states as an overall pattern, it might make sense to update other areas that could reasonably use hover states like links in theme settings. @TheWebTech @ajlaporte do either of you happen to have a good pulse on if adding hover options is a common practice among developers/is this something we could potentially survey in the developer Slack?

jasonnrosa avatar Sep 13 '21 19:09 jasonnrosa

We can poll folks. Overall i think its going to be a complete mixed bag. Themes built and designed specifically for a company's site likely dont offer hover styling controls.

Marketplace providers however likely do offer hover styling controls. Whether its theme wide or they offer granular control at the module level too. I'm not sure. Likely also mixed.

TheWebTech avatar Sep 14 '21 12:09 TheWebTech

Launched a poll: https://hubspotdev.slack.com/archives/CSFGKSHT7/p1631625417041300

TheWebTech avatar Sep 14 '21 13:09 TheWebTech