chore: Responsive Card Section button on cards (WEB-15)

[joshuaow91]

No description provided.

[braydoncoyer]

@joshuaow91

Thoughts on turning these into a reusable button component? This approach would offer several benefits:

1. Enables creating specific tests for the button.
2. Keeps styles confined to the button rather than the card component.

[braydoncoyer]

Does this code change include the arrow next to the button itself?

[braydoncoyer]

nit: We could potentially give this component a more generic name so that it can be used for various functions in the future. As is, the Figma file only shows primary buttons, but I assume that we may have different variants in the future (non-CTA) and scoping buttons to scenarios may be overkill.

Additionally, to make it more reusable, consider simply presenting the buttonText and optional (maybe, ask Danny) icon and allowing the consumer of this component to handle what happens on click (notice the onClick callback prop below).

Perhaps something like this?

export type ButtonVariant = "primary"; // To be extended later on when design calls for additional variants

export interface ButtonProps {
  buttonText: string;
  variant?: ButtonVariant;
  onClick: () => void;
  showIcon?: boolean;
}

export default function Button({
  buttonText,
  onClick,
  variant = "primary",
  showIcon,
}: ButtonProps) {
  const buttonClass = variant === "primary" ? styles["button--primary"] : "";

  return (
    <button className={`${styles.button} ${buttonClass}`} onClick={onClick}>
      {buttonText}
      {showIcon ? <span className={styles.button__icon}> ARROW ICON GOES HERE ></span> : null}
    </button>
  );
}

Leaving this feedback as a nit because I can see the argument of creating another ticket to refactor this to a more generic button component. :)

[joshuaow91]

This is great feedback, i'll implement this!

[braydoncoyer]

Should we create a test file for the new button component and include some basic tests?

[joshuaow91]

button extracted into reusable component, test file also there

[braydoncoyer]

nit and non-blocking: Instead of explicitly setting showIcon={true}, we can simply use showIcon as a prop, and it will be considered true by default.

[braydoncoyer]

If a max-width isn't needed, consider letting the button adjust its size based on the text from the component prop. This can be done by setting appropriate padding, rather than an explicit width here.

[joshuaow91]

this is my usual approach but with tailwind, i was following the guidelines in the original issue. I agree though, i'll make this change!

[braydoncoyer]

If we're justing using left padding here, consider removing the shorthand and using padding-left: 5px.

[joshuaow91]

no problem, thank you

[braydoncoyer]

I'm not sure this test is actually doing what you think it may be doing. Whether the primary variable is applied or not, this button element will also have a class applied to it, correct? This test may be giving you a false-positive.

[joshuaow91]

I wasn't sure if this was even the correct approach. Also wasn't sure how to make this more flexible and dynamic if at even possible, otherwise wouldn't it require a separate test for each variant?

[braydoncoyer]

It would need a new test for each variant - generally, though, that's not a bad thing. I think it provides value to know if a specific variant suddenly fails or breaks. Buttons typically have 2-3 variants. Adding a couple of extra tests to our testing suite is manageable imho!

[joshuaow91]

is this appropriate? it's telling me it can't find the class

test('has the correct class when variant is primary', () => { render(<Button buttonText='Primary Button' variant='primary' />); const buttonElement = screen.getByText(/primary button/i); expect(buttonElement).toHaveClass(styles['button--primary']); });

[joshuaow91]

Having trouble getting the tests to mock / pick up the import styles in the test file. used a snapshot test for the primary button