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

New components/button #282

Merged
merged 24 commits into from
Apr 15, 2023
Merged

New components/button #282

merged 24 commits into from
Apr 15, 2023

Conversation

sebherrerabe
Copy link
Contributor

@sebherrerabe sebherrerabe commented Mar 19, 2023

Hi team,

I've added a ButtonComponent with props for size, variant, and HTMLElement, as well as a default and two additional variants for styling. I've also added a story to the Storybook for testing.

Props:

size: string (optional) - The size of the button. Accepts "small" or "medium". Default is "medium".
variant: string (optional) - The styling variant of the button. Accepts "default", "primary", or "secondary". Default is "default".
HTMLElement: string (optional) - The type of HTML element to use for the button. Accepts "button" or "a". Default is "button".
Let me know if you have any feedback or questions.

closes #264

Button.-.Default.Storybook.et.3.pages.de.plus.-.Travail.Microsoft.Edge.2023-03-19.16-35-43.mp4

Storybook - Test runner

Since I installed the test runner provided by Storybook, we will have to update the documentation of the project:

Storybook Tests
To run the Storybook tests, please make sure to run the following command:

npm run test:sb

In addition, if you encounter any errors related to Playwright, you might need to install its dependencies by running:

npx playwright install-deps

This will ensure that the Storybook tests run properly.

@sebherrerabe sebherrerabe added the enhancement New feature or request label Mar 19, 2023
@sebherrerabe sebherrerabe self-assigned this Mar 19, 2023
Copy link
Contributor

@KevinTss KevinTss left a comment

Choose a reason for hiding this comment

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

Very nice work 👏 Thank your for the contribution, it's a really cool start

I'm thinking about couple of things to make it better:

  1. When I click on the button it would be nice to see a different color, we might need to use polished to add a slightly darker color when to mouse click on it (see videos below)
  2. We might want to control the outline and use the primary color in order to have a more consistant visual when navigate with keyboard (Tab ->|)
  3. It would nice to explore a bit testing as well since they have log of testing libraries we can try. Of course if you don't feel explore that part no worries I can look into it later on.
Screen.Recording.2023-03-19.at.21.37.13.mov

Example with element-ui

Screen.Recording.2023-03-19.at.21.40.04.mov

children?: ReactNode
size?: "small" | "medium"
variant?: "default" | "primary" | "secondary"
HTMLElement?: "button" | "a"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can omit this and use the as from styled-component instead. In order to make sure to not allow anything else apart the 2 allowed tags you can lock that with typescript types

Then in that case you won't need any React component but only Styled component

Copy link
Contributor Author

@sebherrerabe sebherrerabe Mar 24, 2023

Choose a reason for hiding this comment

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

  • Use "as" prop instead of using 2 styled components

<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link
href="https://fonts.googleapis.com/css2?family=Poppins:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;0,800;0,900;1,100;1,200;1,300;1,400;1,500;1,600;1,700;1,800;1,900&display=swap"
Copy link
Contributor

Choose a reason for hiding this comment

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

I might advice to not import all the weights in order to keep it as light as possible.
Maybe we can start with 400 (for default) & 500 (for primary and secondary) for now then see it we need something else later.

Copy link
Contributor Author

@sebherrerabe sebherrerabe Mar 24, 2023

Choose a reason for hiding this comment

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

  • Reduce quantity of font weights being imported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I explored and implemented the tools offered by Storybook. In the following list, you can check which ones I implemented and why. Also, notice that there were some tools that I didn't implement.

Test runner IMPLEMENTED. I implemented it because it lets you run all the tests that you wrote in the play function of every story. It will be interesting to add it to the building process in GitHub and in prod. Check the PR description to learn how to run it.

Visual tests I didn't implement since it requires registering for a special token, and I don't have the credentials of our team to do such a thing. Also, I'm not sure how useful it will be for this particular component. TO BE DISCUSSED.

Accessibility tests IMPLEMENTED. I implemented it since it was easy to implement, and it might be really helpful for the dev experience.

Interaction tests IMPLEMENTED. I implemented it since this is the core of the Storybook testing approach. Specifically for the button component, I focused on the styles of every variant.

Coverage tests IMPLEMENTED. Coverage tests are useful because they ensure that the code is thoroughly tested, identify areas where tests are lacking, and can help optimise code.

Snapshot tests DIDN'T IMPLEMENT. It requires installing the JEST testing library.

Import stories in other tests DIDN'T IMPLEMENT. Since it's more related to test libraries, and my goal wasn't to write Cypress tests for now.

@KevinTss KevinTss merged commit 9025d0f into main Apr 15, 2023
@KevinTss KevinTss deleted the new-components/button branch April 15, 2023 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component - Button
2 participants