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

CTA: Add Center Top/Button Button Alignments #1643

Merged
merged 12 commits into from
Dec 5, 2022
Merged

Conversation

AlexGStapleton
Copy link
Member

Resolve #1637

@AlexGStapleton AlexGStapleton self-assigned this Oct 23, 2022
@AlexGStapleton AlexGStapleton marked this pull request as draft October 23, 2022 16:55
@AlexGStapleton
Copy link
Member Author

AlexGStapleton commented Oct 23, 2022

@Misplon What are your thoughts on adding an optional (due to the pre-existing nature of the widget) responsive collapse for the left and right alignments? We could call it... say, Mobile Button Layout (could also introduce a Tablet) and provide the following options:

  • Standard/desktop Alignment.
  • Above text.
  • Below text.

I think it wouldn't be a bad idea to introduce it in this PR (unless it was to be merged really quickly) as it'll benefit from the top alignment and the LESS cleanup.

@AlexGStapleton AlexGStapleton marked this pull request as ready for review October 23, 2022 17:03
@Misplon
Copy link
Member

Misplon commented Oct 29, 2022

Thanks, mate; yeah, agreed.

Let's re-think this slightly. Can we perhaps rename the setting to something like Alignment? How do you want to do this? Within a Responsive section?

  • For Center Top and Center let's perhaps center the text.
  • Do you want to remove the Mobile Align setting from the Button section? I'm not sure it does anything.

@AlexGStapleton
Copy link
Member Author

Can we perhaps rename the setting to something like Alignment

We can do this, but we'll need to rename the center top/bottom settings as they don't really make sense when the setting is called Alignment.

Within a Responsive section?

I opted to add a single setting below the alignment setting. If we were to introduce other responsive settings a dedicated section may be worth it but with only a single one it's hard to justify.

@AlexGStapleton
Copy link
Member Author

This PR could resolve #457 in its current state.

@Misplon Misplon merged commit 8ce2035 into develop Dec 5, 2022
@Misplon Misplon deleted the cta-center-alignments branch December 5, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

CTA: Introduce New Button Align Settings
2 participants