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

fix: Align button content to the center #243

Merged
merged 9 commits into from
May 23, 2024

Conversation

gabyzif
Copy link
Contributor

@gabyzif gabyzif commented May 23, 2024

Whenever a button has an icon larger than the label's font size, it becomes misaligned

image

Currently, we are adding an icon through the icon prop. While version 5.17 (we are currently in 5.13) introduces iconPosition this prop only allows switching the icon between the left and right sides, not centering it.

Screenshot 2024-05-23 at 9 24 08 PM Screenshot 2024-05-23 at 9 24 21 PM

Since we are adding it though props and not in the children, we can't use a Flex container to make the button aligned, and we can't let the children handle the alignment either. Here is a code snippet from ant's implementation on the icon/children. The rest of the code you can find it here:

const genButtonContent = (iconComponent: React.ReactNode, kidsComponent: React.ReactNode) =>
  iconPosition === 'start' ? (
    <>
      {iconComponent}
      {kidsComponent}
    </>
  ) : (
    <>
      {kidsComponent}
      {iconComponent}
    </>
  );

As a workaround for this problem, I set up a class that makes the button flex and center aligned only when it has an icon

The button with the fix:
Screenshot 2024-05-23 at 4 26 06 PM

@@ -0,0 +1,4 @@
.button {
Copy link
Collaborator

Choose a reason for hiding this comment

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

question I'm suprised that this is happening. My hunch is that maybe we've missed a Design Token. Have those been investigated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bottom button is a raw ant component without our theme on top nor the new class.

Screenshot 2024-05-23 at 5 01 23 PM

Copy link
Collaborator

@tibuurcio tibuurcio left a comment

Choose a reason for hiding this comment

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

I was obligated to left a comment by github

image


export interface IButtonProps extends AntButtonProps {}

export const Button = (props: IButtonProps) => {
return (
<ConfigProvider>
<AntButton {...props}>{props.children}</AntButton>
<AntButton {...props} className="aquarium-button">
Copy link
Collaborator

Choose a reason for hiding this comment

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

hum, um, yea can we do this in a different way? why not use a utility class, or a Flex component?

also, why do we need every single button from the aquarium to always be flex? shouldnt it def be a props.children level concern?

if this is fixed in 5.17, might it be better to find an update path instead of patch a hack with display?

@@ -0,0 +1,4 @@
.aquarium-button--aligned-center {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit hey its me again 〰️
could prefer present tense here, align over aligned, its less typing too 😉

but really per the Style Guide, modifier selectors should only be used in conjunction with the unmodified class. this ensures a higher specificity and that the modifier will "always override" the base class

in this case, it would be like .aquarium-button.aquarium-button--aligned-center {}

which would be correct doesnt really make a ton of sense since we dont have any other .aquarium-buttons or rules for a base .aquarium-button

peronally for things like this i'm more a fan of utility classes

we have a place for more in the aquarium

could be something like

<AntButton {...props} className={props.icon ? 'u-display-flex u-align-items-center' : ''}>

Copy link
Contributor Author

@gabyzif gabyzif May 23, 2024

Choose a reason for hiding this comment

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

I was literally changing it for the utility class 🙏 . I'm also big fan of utility classes

src/utils/utils.css Outdated Show resolved Hide resolved
Copy link
Collaborator

@tibuurcio tibuurcio left a comment

Choose a reason for hiding this comment

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

Good job on the investigation, evolution of context and alignment seeking in this PR :)

@gabyzif gabyzif merged commit ca0901d into feat/icons-integration May 23, 2024
9 of 11 checks passed
mparticle-automation added a commit that referenced this pull request May 23, 2024
# [1.15.0-icons-integration.3](v1.15.0-icons-integration.2...v1.15.0-icons-integration.3) (2024-05-23)

### Bug Fixes

* Align button content to the center ([#243](#243)) ([ca0901d](ca0901d))
@mparticle-automation
Copy link
Collaborator

🎉 This PR is included in version 1.15.0-icons-integration.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gabyzif gabyzif mentioned this pull request May 24, 2024
@gabyzif gabyzif deleted the fix/button-aligment branch August 13, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants