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

Loading in button #1788

Merged
merged 9 commits into from
Aug 1, 2024
Merged

Loading in button #1788

merged 9 commits into from
Aug 1, 2024

Conversation

mishramonalisha76
Copy link
Collaborator

Pull Request Template

Ticket Number

Description

  • Problem/Feature: Added loading state in button

Type of Change

  • Bug fix
  • New feature
  • Code refactor
  • Documentation update
  • Other (please describe):

Checklist

  • Quick PR: Is this a quick PR? Can be approved before finishing a coffee.
    • Quick PR label added
  • Not Merge Ready: Is this PR dependent on some other PR/tasks and not ready to be merged right now.
    • DO NOT Merge PR label added

Frontend Guidelines

Build & Testing

  • No errors in the build terminal
  • Engineer has tested the changes on their local environment
  • Engineer has tested the changes on deploy preview

Screenshots/Video with Explanation

image image

Additional Context

Review & Approvals

  • Self-review completed
  • Code review by at least one other engineer
  • Documentation updates if applicable

Notes

Copy link

github-actions bot commented Aug 1, 2024

In the code provided there are a few mistakes and improvements that can be made:

  1. In Button.tsx file:
  • There is a typo in the comment: /* Custom CSS applied via styled component css prop */ should be corrected to /* Custom CSS applied via styled component css prop */
  • In the getButtonVariantStyles function, there are missing closing brackets and semicolons for the CSS properties inside each case statement. Each property should be enclosed in curly braces {} and each case block should be properly closed with }.
  • The 'tertiary' case in getButtonVariantStyles function is missing the closing curly brace '}' at the end.
  • In the 'tertiary' and 'danger' secondary case statements, there is a typo 'backrgound' which should be corrected to 'background'.
  • In getButtonSizeStyles function:
    • There are missing backticks for the template literals used within the css blocks for 'extraSmall', 'small' and 'medium' cases.
    • For the 'small' case, the closing curly brace after the line height: 24px; is missing.
    • For the 'medium' case, the closing curly brace after the line width: 48px; is missing.
  1. In Spinner.tsx file:
  • In the Container styled component, the :before and :after pseudo-elements should have a comma between them to be properly selected. So, :before,:after { should be corrected to :before, :after {.
  • There is a missing closing curly brace '}' at the end of the Container styled component.
  • In the return statement of the Spinner functional component, the <Container> component is missing the closing tag </Container>.

Please correct these issues in the respective files.

If all the mentioned mistakes are corrected, the code is expected to be error-free and no logic issues are observed.

All looks good.

Copy link

github-actions bot commented Aug 1, 2024

In the code provided, I found some mistakes and typos that need to be addressed:

  1. In Button.tsx:

    • Typo in the comment in the ButtonProps interface: "by Box" should be "by Button".
    • Typo in the property name backrgound should be background in getButtonVariantStyles.
    • Missing closing braces in multiple cases for :active, :focus-visible, and :disabled within the switch cases in getButtonVariantStyles.
    • Typo in backrgound should be background in getButtonVariantStyles.
    • Typo in backrgound should be background in multiple switch cases of getButtonVariantStyles.
    • Typo in backrgound should be background in multiple switch cases of getButtonVariantStyles.
    • Typo in .icon-text > span should be .icon-only > span in getButtonSizeStyles.
    • Typo in leading-trim and text-edge should be letter-spacing and text-align, respectively, in getButtonSizeStyles.
  2. In Button.utils.ts:

    • Typo in backrgound should be background in multiple switch cases of getButtonVariantStyles.
    • Typo in size: ButtonSize in getButtonSizeStyles should be size?: ButtonSize.
  3. In Spinner.tsx:

    • Missing import of keyframes from styled-components.
    • Missing keyframes definition for the spin animation.
    • Missing closing backticks for the CSS property value in const Container.
    • Typo in getSpinnerStrokeWidth(size) should use the correct function call without curly braces.
    • Missing closing braces for the :before and :after pseudo-elements CSS in Container.
    • Missing semicolon at the end of the border-color property in variant.

Overall, the code seems to have multiple typos and mistakes that need to be corrected.

I recommend making the necessary adjustments and correcting the typos before stating 'All looks good'.

Copy link

github-actions bot commented Aug 1, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-08-01 16:25 UTC

src/blocks/button/Button.tsx Outdated Show resolved Hide resolved
src/blocks/button/Button.tsx Outdated Show resolved Hide resolved
src/blocks/button/Button.tsx Outdated Show resolved Hide resolved
src/blocks/button/Button.tsx Show resolved Hide resolved
src/blocks/button/Button.tsx Show resolved Hide resolved
src/blocks/button/Button.tsx Outdated Show resolved Hide resolved
src/blocks/button/Button.utils.ts Outdated Show resolved Hide resolved
src/blocks/button/Button.utils.ts Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Aug 1, 2024

In the Button.tsx file:

  1. There seems to be an issue with the conditional rendering of spinner in the component. It should be {loading && <Spinner />}, not {loading ? <Spinner /> : null}.

In the Button.utils.ts file:

  1. Inside the getButtonVariantStyles function, there are syntax errors in multiple cases. For example, missing closing curly braces, missing semicolons, and incorrect indentation.
  2. In the getButtonSizeStyles function, the condition check for size should use if...else if...else, but it's missing the else if part.
  3. Inside the getButtonSizeStyles function, there are missing closing curly braces in the returned values.
  4. Inside the getButtonSizeStyles function, the logic for size === 'small' and size === 'medium' is not enclosed in curly braces, causing a syntax error.

Please make the necessary corrections as mentioned above.

All looks good.

Copy link

github-actions bot commented Aug 1, 2024

In the first file (Button.tsx), there are a few issues and improvements needed:

  1. In the StyledButton component, the getButtonVariantStyles function is called with loading!, but it should be loading || false to prevent potential runtime errors. It should be updated to:
${({ variant, loading }) => getButtonVariantStyles(variant || 'primary', loading || false)}
  1. In the same StyledButton component, the closing tag for the margin property inside [role='spinner'] is missing. It should be added:
[role='spinner'] {
    margin: 6.667px; /* Assuming this line is correct */
}
  1. The ':before,:after' declarations inside the getButtonVariantStyles function's case 'primary' are incorrect. The colon (:) should be attached to the :before and :after and not separated by a comma. It should be:
:before, :after {
    background: var(--components-button-primary-icon-default);
}
  1. In the same function getButtonVariantStyles, inside the case 'tertiary', there's inconsistency with the declarations and some incorrect CSS properties applied within the cases. You need to review and align the CSS properties and classes.
  2. In the getButtonSizeStyles function within the same file, the missing conditions for other size options (extraSmall, small) should be added similarly to the existing conditions for medium. Additionally, the trailing of if (size === 'small') { block is missing. It should be properly closed before handling the medium size property.
  3. It seems there is an inconsistency with the indentation level of the CSS properties and classes inside the template literals in the getButtonVariantStyles and getButtonSizeStyles functions. Ensure consistent indentation for better readability.

Overall, after addressing the above improvements, the rest of the code seems structured and does what is intended.

In the second file (Button.utils.ts), there seems to be a formatting issue within the getButtonVariantStyles function, particularly in the case 'secondary'. The structure looks correct, but some lines are missing backticks, and there are misplaced semicolons. Ensure all properties are correctly enclosed in backticks and correctly separated with semicolons where necessary.

In the third file (Spinner.tsx), the code looks fine. No issues were found.

If you make the necessary corrections and address the issues mentioned, the code should be in good shape.

Copy link
Collaborator

@rohitmalhotra1420 rohitmalhotra1420 left a comment

Choose a reason for hiding this comment

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

  • We need to fix the spinner border width for different screen sizes.
  • Let's use svg instead of div.
  • Fix the alignment of the spinner in different screen sizes.
  • Removed any additional css we have applied to make the div spinner work.
  • apply an additional check in the button to not change color on press when it's in loading state.

Copy link

github-actions bot commented Aug 1, 2024

In Button.tsx:

  1. In the SpinnerContainer styled component, there is a missing closing curly brace } after padding: 5px;. It should be added to properly close the component definition.
  2. In the StyledButton styled component, the circular CSS is missing a closing curly brace after setting border radius for circular buttons. It should be like this:
    ${({ circular, iconOnly }) => circular && iconOnly && `border-radius: var(--r10);`}
  3. In the StyledButton styled component, there is a missing closing parenthesis in the SpinnerContainer selector. It should be added:
    ${({ loading }) => loading && `opacity: var(--opacity-80);`}
  4. In the getButtonVariantStyles function, the case 'secondary' is missing opening curly braces { for the code block under that case.
  5. In the getButtonVariantStyles function, the case 'tertiary' is missing opening and closing curly braces {} for the code block under that case.
  6. In the getButtonSizeStyles function, there are missing closing curly braces } for the if (size === 'extraSmall') and if (size === 'small') condition blocks. They should be added appropriately to close the code blocks.

In Ellipse.tsx:
7. The Ellipse component is handling props using allProps and then extracting svgProps and restProps, but these are not being used anywhere in the component. Either utilize these props or refactor the component accordingly.

Overall, please check the code comments, CSS styles, and code logic to ensure everything works as intended.

If the suggested changes are made, the code should be good to go.

Copy link

github-actions bot commented Aug 1, 2024

All looks good.

@rohitmalhotra1420 rohitmalhotra1420 merged commit 4c6ca21 into main Aug 1, 2024
2 checks passed
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.

😈 [Feature Enhancement] - Add loading state to button component
2 participants