Skip to content

fix: added height on loading button to show indicator #168

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.

Sign up for GitHub

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

Open
mafeoli wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: added height on loading button to show indicator #168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 20 additions & 24 deletions src/components/Button/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* eslint-disable prettier/prettier */
Copy link

@gemini-code-assist gemini-code-assist bot Jun 6, 2025

Choose a reason for hiding this comment

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

medium

I notice /* eslint-disable prettier/prettier */ was added at the top of the file. While sometimes necessary for specific lines, disabling Prettier for an entire file can lead to inconsistencies and make it harder to maintain a uniform codebase style. Could you clarify the reason for this? Perhaps we could resolve the underlying Prettier conflict that prompted this, or if a specific line is problematic, use a more targeted // eslint-disable-next-line prettier/prettier comment just for that line?

Copy link
Contributor

@mdlucas mdlucas Jun 6, 2025

Choose a reason for hiding this comment

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

@mafeoli melhor deixa habilitado e ajustar a identação


import styled from 'styled-components/native';
import {
ButtonColorType,
Expand Down Expand Up @@ -100,45 +102,38 @@ type TextButtonProps = {
const getTextColor = (props: TextButtonProps): string => {
switch (props.colorVariant) {
case 'primary':
return `${
props.buttonVariant === 'filled'
return `${props.buttonVariant === 'filled'
? brandPrimaryContrast(props)
: brandPrimary(props)
}`;
}`;
case 'secondary':
return `${
props.buttonVariant === 'filled'
return `${props.buttonVariant === 'filled'
? brandSecondaryContrast(props)
: brandSecondary(props)
}`;
}`;
case 'accent':
return `${
props.buttonVariant === 'filled'
return `${props.buttonVariant === 'filled'
? brandAccentContrast(props)
: brandAccent(props)
}`;
}`;
case 'danger':
return `${
props.buttonVariant === 'filled'
return `${props.buttonVariant === 'filled'
? dangerContrast(props)
: dangerMain(props)
}`;
}`;
case 'info':
return `${
props.buttonVariant === 'filled' ? infoContrast(props) : infoMain(props)
}`;
return `${props.buttonVariant === 'filled' ? infoContrast(props) : infoMain(props)
}`;
case 'warning':
return `${
props.buttonVariant === 'filled'
return `${props.buttonVariant === 'filled'
? warningContrast(props)
: warningMain(props)
}`;
}`;
case 'success':
return `${
props.buttonVariant === 'filled'
return `${props.buttonVariant === 'filled'
? successContrast(props)
: successMain(props)
}`;
}`;
default:
return `${brandPrimaryContrast(props)}`;
}
Expand All @@ -148,7 +143,7 @@ type TouchableProps = {
rounded: boolean;
} & ThemeProps;

export const Touchable = styled(TouchableComponent)<TouchableProps>`
export const Touchable = styled(TouchableComponent) <TouchableProps>`
border-radius: ${(props: TouchableProps): any =>
props.rounded ? buttonSize / 2 : buttonRadius(props)}px;
`;
Expand All @@ -172,7 +167,7 @@ export const ButtonWrapper = styled.View<ButtonWrapperProps>`
opacity: ${isDisabled(0.3, 1)};
`;

export const TextButton = styled(TypographyComponent)<TextButtonProps>`
export const TextButton = styled(TypographyComponent) <TextButtonProps>`
letter-spacing: 0.4px;
color: ${isFlat(getBackgroundColor, getTextColor)};
font-weight: bold;
Expand All @@ -183,6 +178,7 @@ export const Loading = styled(LoadingIndicator).attrs({
})`
align-self: center;
width: 55px;
height: 55px
Comment on lines 178 to +181
Copy link

@gemini-code-assist gemini-code-assist bot Jun 6, 2025

Choose a reason for hiding this comment

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

medium

This change sets height: 55px for the Loading component, matching its existing width: 55px. There are a few points to consider here:

  1. Potential Clipping: The ButtonWrapper (which contains this Loading indicator) has a height of 45px (derived from the buttonSize constant on line 54) and also an overflow: hidden style (line 158). This means a 55x55px loading indicator will be clipped by 10px in height (5px from the top and 5px from the bottom, if centered). Is this clipping the intended behavior? If the indicator should be fully visible and fit within the button, its dimensions might need to be closer to buttonSize (45px) or slightly less to account for any internal padding.
  2. Magic Number: The value 55px is used for both width (line 180) and now height. If this dimension is intentional and distinct from buttonSize, could we define it as a constant (e.g., const loadingIndicatorDimension = 55;) at the top of the file for better clarity and maintainability?

`;

type IconProps = {
Expand All @@ -195,7 +191,7 @@ type IconProps = {

export const Icon = styled(DefaultIcon).attrs((props: IconProps) => ({
color: getTextColor(props),
}))<IconProps>`
})) <IconProps>`
margin-right: ${isLeftIcon(smallSpacing, 0)}px;
margin-left: ${isRightIcon(smallSpacing, 0)}px;
`;
Loading