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.

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

feat(IconButton): loading state #2029

Merged
merged 7 commits into from
Mar 25, 2024
Merged

feat(IconButton): loading state #2029

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a00891
feat(IconButton): loading state
SergeyRoyt Mar 20, 2024
be4346f
Merge branch 'master' into sergeyro/feature/icon-button-loading
SergeyRoyt Mar 20, 2024
f9db261
refactoring
SergeyRoyt Mar 21, 2024
ddad5f1
refactor prop description
SergeyRoyt Mar 24, 2024
ce324fb
Merge branch 'master' into sergeyro/feature/icon-button-loading
SergeyRoyt Mar 24, 2024
eec2f09
Merge branch 'master' into sergeyro/feature/icon-button-loading
SergeyRoyt Mar 25, 2024
bea2e65
fix loader selector
SergeyRoyt Mar 25, 2024
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
6 changes: 5 additions & 1 deletion packages/core/src/components/Button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ export interface ButtonProps extends VibeComponentProps {
successText?: string;
/** loading boolean which switches the text to a loader */
loading?: boolean;
/** className which is applied to loader container **/
loaderClassName?: string;
style?: React.CSSProperties;
/** displays the active state */
active?: boolean;
Expand Down Expand Up @@ -120,6 +122,7 @@ const Button: VibeComponent<ButtonProps, unknown> & {
successIcon,
style,
loading: isLoading,
loaderClassName,
active,
activeButtonClassName,
id,
Expand Down Expand Up @@ -345,7 +348,7 @@ const Button: VibeComponent<ButtonProps, unknown> & {
if (loading) {
return (
<button {...buttonProps} key={`${id}-loading`}>
<span className={styles.loader}>
<span className={cx(styles.loader, loaderClassName)}>
<Loader className={styles.loaderSvg} />
<span aria-hidden className={styles.textPlaceholder}>
{buttonContent}
Expand Down Expand Up @@ -405,6 +408,7 @@ Button.defaultProps = {
successText: "",
successIcon: null,
loading: false,
loaderClassName: undefined,
active: false,
marginRight: false,
marginLeft: false,
Expand Down
14 changes: 13 additions & 1 deletion packages/core/src/components/IconButton/IconButton.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,16 @@

.referenceWrapper {
display: inline-flex;
}
}

.loader.loader {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we providing only a partial set of options? Let's give the ability to show the loader for all sizes, is it by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's showing for all of the sizes
We just need an override for the xxs size - 10 instead of 16

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need it anyway? Doesn't get a default size already in Loader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Can you nest instead of using the class twice? It will increase specificity

width: 16px;
height: 16px;
Comment on lines +12 to +13
Copy link
Member

Choose a reason for hiding this comment

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

Do we have variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which variables would you use here?

&.xxs {
width: 10px;
height: 10px;
svg {
display: block;
}
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

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

Why is it necessary? Is it inline by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
It's inline by default, yeah
Without display block it isn't displaying properly

}
}
8 changes: 7 additions & 1 deletion packages/core/src/components/IconButton/IconButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { ComponentDefaultTestId } from "../../tests/constants";
import { backwardCompatibilityForProperties } from "../../helpers/backwardCompatibilityForProperties";
import Button from "../Button/Button";
import { BUTTON_ICON_SIZE, ButtonColor, ButtonType } from "../Button/ButtonConstants";
import { getStyle } from "../../helpers/typesciptCssModulesHelper";
import styles from "./IconButton.module.scss";

export interface IconButtonProps extends VibeComponentProps {
Expand Down Expand Up @@ -86,6 +87,8 @@ export interface IconButtonProps extends VibeComponentProps {
insetFocus?: boolean;
/** Specifies the tab order of an element */
tabIndex?: number;
/** loading boolean which switches the text to a loader */
SergeyRoyt marked this conversation as resolved.
Show resolved Hide resolved
loading?: boolean;
}

const IconButton: VibeComponent<IconButtonProps> & {
Expand Down Expand Up @@ -116,7 +119,8 @@ const IconButton: VibeComponent<IconButtonProps> & {
dataTestId: backwardCompatabilityDataTestId,
"data-testid": dataTestId,
insetFocus = false,
tabIndex
tabIndex,
loading = false
},
ref
) => {
Expand Down Expand Up @@ -198,6 +202,8 @@ const IconButton: VibeComponent<IconButtonProps> & {
style={overrideStyle}
insetFocus={insetFocus}
tabIndex={tabIndex}
loading={loading}
loaderClassName={cx(styles.loader, getStyle(styles, size))}
>
<Icon
icon={icon}
Expand Down