FTP-1383:Design-systems-components-buttons #26

tushar8184 · 2022-10-03T06:54:06Z

vercel · 2022-10-03T06:54:10Z

@tushar8184 is attempting to deploy a commit to the grocery-plus Team on Vercel.

To accomplish this, @tushar8184 needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

skippednote · 2022-10-04T06:06:19Z

components/button/Button.tsx

+        <Image
+          className={iconClasses}
+          // bag-img
+          // src="/vector.png"
+
+          // save-img
+          // src="/save.png"
+
+          // left_arrow-img
+          // src="/left_arrow.png"
+
+          // right_arrow-img


@tushar8184 Please remove the comments.

skippednote · 2022-10-04T06:06:35Z

components/button/Button.tsx

+          // src="/left_arrow.png"
+
+          // right_arrow-img
+          src="/right_arrow.png"


This should be passed as a prop (iconName)

skippednote · 2022-10-04T06:06:55Z

components/button/Button.tsx

+          width={17}
+          height={19.6}


Set these using tailwind classes

skippednote · 2022-10-04T06:07:14Z

components/button/Button.tsx

+      )}
+      <span className={textColor + ' mx-auto ' + textClasses}>{text}</span>
+      {iconRight && (
+        <Image


Instead of the next.js component, lets use

skippednote · 2022-10-04T06:07:28Z

components/button/Button.tsx

+        <Image
+          className={iconClasses}
+          src="/vector.png"
+          width={9.92}
+          height={11.46}
+        />


Same as below

skippednote · 2022-10-04T06:10:05Z

components/button/Button.tsx

-  return <button>{children}</button>;
+const Button = ({
+  variant,
+  text = 'Button',


Let's remove the default value

skippednote · 2022-10-04T06:10:55Z

Please export SVG icons instead.
Please make the tests pass.

vercel · 2022-10-04T12:56:07Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
grocery-plus	✅ Ready (Inspect)	Visit Preview	Oct 4, 2022 at 1:35PM (UTC)

skippednote · 2022-10-07T05:42:05Z

components/button/Button.test.tsx

@@ -1,14 +1,13 @@
 import { render, screen } from '@testing-library/react';


Please revert this file

Made the required changes

skippednote · 2022-10-07T05:44:49Z

components/button/Button.test.tsx

 describe('Test Button component', () => {
  it('renders the component', () => {
-    render(<Button>Test data</Button>);
+    render(<button>Test data</button>);
    const button = screen.getByRole('button');
    expect(button).toBeInTheDocument();
  });
  it('renders the correct child data', () => {
-    render(<Button>Test data</Button>);
+    render(<button>Test data</button>);
    const button = screen.getByRole('button');
    expect(button).toHaveTextContent(/test data/i);
  });


Add more tests to check all variants and other props

Have added one more test to check all the variants.

skippednote · 2022-10-07T05:45:00Z

components/button/Button.tsx

-  return <button>{children}</button>;
+const Button = ({
+  variant,
+  text = '',


let's use children instead

skippednote · 2022-10-07T05:45:13Z

components/button/Button.tsx

-  return <button>{children}</button>;
+const Button = ({
+  variant,
+  text = '',


Text shouldn't have a default value

skippednote · 2022-10-07T05:46:17Z

components/button/Button.tsx

+    <div
+      className={classes + ' ' + extraClasses}
+      onClick={() => (onClick != null ? onClick : null)}
+    >
+      {iconLeft && <img className="w-7" src={iconName} />}
+      <span className={textColor + ' mx-auto ' + textClasses}>{text}</span>
+      {iconRight && <img className="w-7" src={iconName} />}
+    </div>


Let's use the Icon component here

skippednote · 2022-10-07T05:46:28Z

@types/button.types.ts

@@ -0,0 +1,10 @@
+export type ButtonType = {
+  variant: string;


Use the values instead.

You need to list all the variant names here instead of string

skippednote · 2022-10-10T05:09:29Z

components/button/Button.test.tsx

+  it('renders the component based on variant', () => {
+    render(<Button variant="primary" />);
+    const button = screen.getByRole('button');
+    expect(button).toHaveClass('bg-green-primary h-12');
+  });
+  it('renders the component based on variant', () => {
+    render(<Button variant="primary-small" />);
+    const button = screen.getByRole('button');
+    expect(button).toHaveClass('bg-green-primary h-9');
+  });
+  it('renders the component based on variant', () => {
+    render(<Button variant="secondary" />);
+    const button = screen.getByRole('button');
+    expect(button).toHaveClass('bg-red-primary h-12');
+  });
+  it('renders the component based on variant', () => {
+    render(<Button variant="secondary-small" />);
+    const button = screen.getByRole('button');
+    expect(button).toHaveClass('bg-red-primary h-9');
+  });
+  it('renders the component based on variant', () => {
+    render(<Button variant="tertiary" />);
+    const button = screen.getByRole('button');
+    expect(button).toHaveClass('bg-light-off h-12');
+  });
+  it('renders the component based on variant', () => {
+    render(<Button variant="tertiary-small" />);
+    const button = screen.getByRole('button');
+    expect(button).toHaveClass('bg-light-off h-9');
+  });


Please add the variant name in the it string

skippednote · 2022-10-10T05:09:47Z

@types/button.types.ts

@@ -0,0 +1,10 @@
+export type ButtonType = {
+  variant: string;


You need to list all the variant names here instead of string

components/button/Button.test.tsx

skippednote · 2022-10-12T05:28:30Z

Please remove the icons in this pr

skippednote · 2022-10-12T05:25:57Z

components/button/Button.test.tsx

+  it('display an alert onClick', () => {
+    render(
+      <Button
+        variant="primary"
+        text="test data"
+        onClick={(event) => {
+          event.currentTarget.classList.add('clicked');
+        }}
+      />,
+    );
+    const button = screen.getByRole('button');
+    fireEvent.click(button);
+    expect(button).toHaveClass('clicked');
+  });
+


Can we move this test to the bottom

move to the bottom

skippednote · 2022-10-12T05:26:20Z

components/button/Button.test.tsx

    const button = screen.getByRole('button');
    expect(button).toHaveTextContent(/test data/i);
  });
+
+  it('display an alert onClick', () => {


Suggested change

it('display an alert onClick', () => {

it('add class on onClick', () => {

added 'add class on'

skippednote · 2022-10-12T05:27:26Z

components/button/Button.tsx

+  textClasses,
+  onClick,
+}: ButtonType) => {
+  let classes = '';


Suggested change

let classes = '';

let classes = defaultClasses';

skippednote · 2022-10-12T05:27:41Z

components/button/Button.tsx

+  const defaultClasses =
+    'flex items-center w-full px-4 py-3 rounded-xs cursor-pointer';
+  if (variant === 'primary') {
+    classes = `${defaultClasses} bg-green-primary h-12`;


Suggested change

classes = `${defaultClasses} bg-green-primary h-12`;

classes += ` bg-green-primary h-12`;

skippednote suggested changes Oct 4, 2022

View reviewed changes

vercel bot deployed to Preview October 4, 2022 12:56 View deployment

vercel bot deployed to Preview October 4, 2022 13:35 View deployment

skippednote reviewed Oct 7, 2022

View reviewed changes

skippednote suggested changes Oct 7, 2022

View reviewed changes

skippednote suggested changes Oct 10, 2022

View reviewed changes

skippednote suggested changes Oct 12, 2022

View reviewed changes

tushar patel added 7 commits October 12, 2022 14:35

FTP-1383:design-systems-components-buttons

69e5af4

FTP-1383:design-systems-components-buttons

651546c

FTP-1383:design-systems-components-buttons

ebe113b

FTP-1383:design-systems-components-buttons

59a823e

FTP-1383:design-systems-components-buttons

5308353

FTP-1383:design-systems-components-buttons

4dcf012

FTP-1383:design-systems-components-buttons-changes

7f37c87

tushar8184 force-pushed the FTP-1383-design-systems-components-buttons branch from 7f1f2b6 to 7f37c87 Compare October 12, 2022 09:12

skippednote merged commit 598b6a7 into main Oct 13, 2022

skippednote deleted the FTP-1383-design-systems-components-buttons branch October 13, 2022 04:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FTP-1383:Design-systems-components-buttons #26

FTP-1383:Design-systems-components-buttons #26

tushar8184 commented Oct 3, 2022

vercel bot commented Oct 3, 2022

skippednote Oct 4, 2022

skippednote Oct 4, 2022

skippednote Oct 4, 2022

skippednote Oct 4, 2022

skippednote Oct 4, 2022

skippednote Oct 4, 2022

skippednote commented Oct 4, 2022

vercel bot commented Oct 4, 2022 •

edited

skippednote Oct 7, 2022

tushar8184 Oct 7, 2022

skippednote Oct 7, 2022

tushar8184 Oct 7, 2022

skippednote Oct 7, 2022

tushar8184 Oct 7, 2022

skippednote Oct 7, 2022

tushar8184 Oct 7, 2022

skippednote Oct 7, 2022

skippednote Oct 7, 2022

tushar8184 Oct 7, 2022

skippednote Oct 10, 2022

skippednote Oct 10, 2022

skippednote Oct 10, 2022

skippednote commented Oct 12, 2022

skippednote Oct 12, 2022

tushar8184 Oct 12, 2022

skippednote Oct 12, 2022

tushar8184 Oct 12, 2022

skippednote Oct 12, 2022

skippednote Oct 12, 2022

		@@ -1,14 +1,13 @@
		import { render, screen } from '@testing-library/react';

	it('display an alert onClick', () => {
	it('add class on onClick', () => {

	classes = `${defaultClasses} bg-green-primary h-12`;
	classes += ` bg-green-primary h-12`;

FTP-1383:Design-systems-components-buttons #26

FTP-1383:Design-systems-components-buttons #26

Conversation

tushar8184 commented Oct 3, 2022

vercel bot commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skippednote commented Oct 4, 2022

vercel bot commented Oct 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skippednote commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Oct 4, 2022 •

edited