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: added button component #8264

Closed
wants to merge 1 commit into from
Closed

feat: added button component #8264

wants to merge 1 commit into from

Conversation

BartoszJarocki
Copy link
Contributor

Description

This PR introduces a reusable button component that could render as button, a or other html tags. It attempts to reduce number of duplicate button components we have, while also keeping consistency and accessibility features.

Demo

[If possible, please include a screenshot or gif/video, it'll make it easier for reviewers to understand the scope of the changes and how the change is supposed to work. If you're introducing something new or changing the existing patterns, please share a Loom and explain what decisions you've made and under what circumstances]

Testing scenarios

[Please list all the testing scenarios a reviewer has to check before approving the PR]

  • Scenario A

    • Step 1
    • Step 2...
  • Scenario B

    • Step 1
    • Step 2....

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label One Review Required if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog


//TODO: implement proper styles
const TYPE_STYLES: Record<Type, string> = {
primary: 'bg-blue-500 hover:bg-blue-600 text-white',
Copy link
Member

Choose a reason for hiding this comment

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

Question: what are the pros/cons of putting these styles in a lookup table like this vs. creating a tailwinds class e.g. btn-primary? https://daisyui.com/components/button/ In other words, when should a component be a tailwinds class vs. when should it be a React component?

Looking for strong opinions, I don't know enough to form an opinion of my own yet!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strong opinion - it should be a component, not a css class. While this is not as black & white for a button, it's clear for more complex html structures.

Here's how I see it

Pros:

  1. Handling complex logic - it's easier to add more complex logic like loading state, having multiple variants
  2. Colocating styles and markup - that's where Tailwind shines the most for me; you can easily see styling, the html structure in the same place
  3. Easier to write <Button variant="primary" /> than <button className="btn-primary" />
  4. Easier refactoring - changing prop name will automatically update all usages, while updating the class name will be a manual/semi-manual work
  5. Feels more natural - having component framework in place and creating CSS abstractions feels off

Cons:

  1. May be an overkill if you assume button won't change and it'll always render the same thing

Here's Tailwind team take on this:
https://tailwindcss.com/docs/reusing-styles#extracting-components-and-partials

Copy link
Member

Choose a reason for hiding this comment

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

i like it! if there's a sensible place to put this opinion in a readme (maybe /components/README.md?) would love to see this opinion live on so we can use it as a razor while building out other components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkrick I'll create a PR with README.md (maybe in client root dir?) where I can copy paste some stuff from the notion doc I created before the migration started, add this opinion and more stuff that I learned while seeing other people use tailwind, how does it sound?

Copy link
Member

Choose a reason for hiding this comment

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

perfect!

@mwermuth
Copy link
Contributor

Looked at during backlog grooming. @BartoszJarocki is this still ongoing or should we close the PR?

@BartoszJarocki
Copy link
Contributor Author

ongoing!

@BartoszJarocki
Copy link
Contributor Author

closing in favor of #9268 where the base component was added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants