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 typings for <Button> #3080

Merged
merged 3 commits into from
May 31, 2024

Conversation

bradenmacdonald
Copy link
Contributor

@bradenmacdonald bradenmacdonald commented May 22, 2024

Description

This adds typescript types for <Button> as well as <ButtonGroup> and <ButtonToolbar>.

I tested it against various usages of <Button> in frontend-app-course authoring, as well as all the usages in this repo itself, and couldn't find any issues.

Deploy Preview

https://deploy-preview-3080--paragon-openedx.netlify.app

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please request an a11y review for the PR in the #wg-paragon Open edX Slack channel.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 22, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented May 22, 2024

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

Copy link

netlify bot commented May 22, 2024

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 66f85cb
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/665779cd5beec7000806755b
😎 Deploy Preview https://deploy-preview-3080--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bradenmacdonald bradenmacdonald marked this pull request as draft May 22, 2024 18:19
Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.20%. Comparing base (48b8635) to head (66f85cb).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3080      +/-   ##
==========================================
+ Coverage   93.19%   93.20%   +0.01%     
==========================================
  Files         249      249              
  Lines        4348     4359      +11     
  Branches     1000     1036      +36     
==========================================
+ Hits         4052     4063      +11     
+ Misses        293      290       -3     
- Partials        3        6       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bradenmacdonald bradenmacdonald marked this pull request as ready for review May 27, 2024 21:55
render(<Button as={Hyperlink} destination="https://www.poop.com/💩">Button</Button>);
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const ref = (_current: HTMLAnchorElement) => {}; // Check typing of a ref - should not show type errors.
render(<Button as={Hyperlink} ref={ref} destination="https://www.poop.com/💩">Button</Button>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shows an example of how passing as={Hyperlink} changes the types of various other props like ref to match.

@@ -30,7 +30,7 @@ describe('<Button />', () => {

it('renders with props iconAfter and size', () => {
const tree = renderer.create((
<Button iconAfter={Close} size="md">Button</Button>
<Button iconAfter={Close} size="sm">Button</Button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed this to sm for some variation - too many of the tests were just using the same md size.

import BaseButton, { type ButtonProps as BaseButtonProps } from 'react-bootstrap/Button';
import BaseButtonGroup, { type ButtonGroupProps as BaseButtonGroupProps } from 'react-bootstrap/ButtonGroup';
import BaseButtonToolbar, { type ButtonToolbarProps } from 'react-bootstrap/ButtonToolbar';
import { type BsPrefixRefForwardingComponent as ComponentWithAsProp } from 'react-bootstrap/esm/helpers';
Copy link
Member

@adamstankiewicz adamstankiewicz May 29, 2024

Choose a reason for hiding this comment

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

[question] Given this pattern of importing BsPrefixRefForwardingComponent (and BsPrefixProps, see #3082), I wonder if there's a more common/global location to define these types when renaming them so each individual component doesn't need to do the same re-naming repeatedly (e.g., risking different naming across components).

For example, Hyperlink (in #3082) will also rely on this pattern so it'd be nice to only rename it in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good idea and I'm happy to move it somewhere shared. Do you have a place in mind or want me to make something up?

Copy link
Contributor

Choose a reason for hiding this comment

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

First thing that came to my mind was to rename https://github.com/openedx/paragon/tree/master/src/utils/propTypes to src/utils/types and then put it in there, but I'm very open to any other ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I didn't rename propTypes for now since I want to distinguish them from types (but feel free to do that later if you want). But otherwise I did this: 66f85cb

I took the opportunity to add lots of documentation to explain how these types work (since the Bootstrap types are pretty obscure), and added in some type-only tests to verify that my understanding of them is correct (and support any future refactors).

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

This is wonderful! Thank you!

@brian-smith-tcril
Copy link
Contributor

I believe codecov is stuck, I'm going to merge this now instead of waiting for that check

image

@brian-smith-tcril brian-smith-tcril merged commit 4abe68b into openedx:master May 31, 2024
9 of 10 checks passed
@openedx-webhooks
Copy link

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@openedx-semantic-release-bot

🎉 This PR is included in version 22.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bradenmacdonald
Copy link
Contributor Author

Thanks @brian-smith-tcril !

@bradenmacdonald bradenmacdonald deleted the type-button branch May 31, 2024 18:38
PKulkoRaccoonGang pushed a commit that referenced this pull request Jul 7, 2024
* feat: add typings for <Button>

* chore: bump @types/react and @types/react-dom

* feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
PKulkoRaccoonGang pushed a commit that referenced this pull request Jul 22, 2024
* feat: add typings for <Button>

* chore: bump @types/react and @types/react-dom

* feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
PKulkoRaccoonGang pushed a commit that referenced this pull request Aug 5, 2024
* feat: add typings for <Button>

* chore: bump @types/react and @types/react-dom

* feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
PKulkoRaccoonGang pushed a commit that referenced this pull request Aug 5, 2024
* feat: add typings for <Button>

* chore: bump @types/react and @types/react-dom

* feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U released
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants