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

Add CTA functionality to button #381

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

TanyaScales
Copy link
Member

@TanyaScales TanyaScales commented Sep 9, 2021

  • 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)
  • 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

- Adjusting visibility of style fields when CTA is selected
@jasonnrosa
Copy link
Member

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.

@TanyaScales
Copy link
Member Author

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.

@jasonnrosa
Copy link
Member

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?

@TheWebTech
Copy link
Contributor

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
Copy link
Contributor

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

@jasonnrosa jasonnrosa marked this pull request as draft October 25, 2022 18:56
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.

5 participants