cms-theme-boilerplate
cms-theme-boilerplate copied to clipboard
Add CTA functionality to button
- 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
Checklist
- [x] I have read the CONTRIBUTING document.
- [x] My code follows the style guide requirements.
- [x] I have thoroughly tested my change.
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 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.
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?
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.
Launched a poll: https://hubspotdev.slack.com/archives/CSFGKSHT7/p1631625417041300