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

feat: add Callout component #240

Merged
merged 6 commits into from
Sep 13, 2024
Merged

Conversation

huyenltnguyen
Copy link
Member

@huyenltnguyen huyenltnguyen commented Jul 19, 2024

Checklist:

This PR adds Callout component to the library (mentioned in #132).

This Callout component allows us to use visual cue to draw users attention to important information, while not disrupting screen reader users workflow.

Notes on the implementation:

  • This Callout component is visually identical to Alert, but it doesn't have a role attribute
  • I'm keeping Callout and Alert as separate components, even though their implementation is almost the same.
    • The similarity in the component display is because we've never had them separated, but that could change now that we start thinking of the two as standalone components. So the split allows us to update the component API more easily in the future (say we could Callout could have a different set of variant, or it could have a title and/or an icon).
    • Part of the rationale is I also want to avoid the situation we have with Button (#211). WET code is better sometimes.

Screenshot

Screenshot 2024-07-19 at 17 12 47

@huyenltnguyen huyenltnguyen marked this pull request as ready for review July 19, 2024 10:36
@huyenltnguyen huyenltnguyen requested a review from a team as a code owner July 19, 2024 10:36
Comment on lines +18 to +35
// @ts-expect-error - Callout does not accept `role`
<Callout variant="success" role="alert">
Hello World
</Callout>;

// @ts-expect-error - Callout does not accept `role`
<Callout variant="info" role="alert">
Hello World
</Callout>;

// @ts-expect-error - Callout does not accept `role`
<Callout variant="warning" role="alert">
Hello World
</Callout>;

// @ts-expect-error - Callout does not accept `role`
<Callout variant="danger" role="alert">
Hello World
Copy link
Member

Choose a reason for hiding this comment

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

These could use a description. ex. it('check something', ()=>

Copy link
Member Author

Choose a reason for hiding this comment

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

So we have two types of test in here:

  • Component functionality tests, which are in the describe / it blocks and run with Jest
  • Type tests, which are to ensure non-allowed props aren't passed in. The tests are run by TypeScript compiler (technically TypeScript doesn't run any special tests, it does throw typecheck errors if we pass in the prohibited props, but // @ts-expect-error helps suppress the errors)

We can put these tests inside an it block, but it's a little weird as the type tests aren't related / powered by Jest. Also, we can't write an assertion (in this case, I guess expect(...).toThrow()) for the it blocks because Jest doesn't know about the TS errors.

I added a comment next to each @ts-expect-error, hopefully that makes the purpose of each type test a little more clear?

@ahmaxed
Copy link
Member

ahmaxed commented Aug 16, 2024

Looking great. The component could use a one pixel border with the same color as the font.
Next step would be flipping the colors for light/dark, so they would get similar amount of contrast comparing to the surrounding elements on the layout.

@ahmaxed ahmaxed added the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label Sep 10, 2024
@huyenltnguyen
Copy link
Member Author

I played a bit with the color switching, and I'm not really happy with the result :/

On dark theme, I think the current combination works better than flipping them:

text green70 + bg green05 text green05 + bg green70
Screenshot 2024-09-12 at 01 10 37 Screenshot 2024-09-12 at 01 10 20

@huyenltnguyen huyenltnguyen added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels Sep 11, 2024
@ahmaxed
Copy link
Member

ahmaxed commented Sep 13, 2024

You are right. Perhaps we could try a color with a low alpha value and see it looks.

@ahmaxed ahmaxed merged commit d3c243e into freeCodeCamp:main Sep 13, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants