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

fix(Offcanvas): make types match existing direction prop #2512

Merged
merged 1 commit into from May 27, 2022

Conversation

BenJenkinson
Copy link
Contributor

@BenJenkinson BenJenkinson commented Apr 29, 2022

Fixes #2511

@crathor
Copy link

crathor commented Apr 29, 2022

@BenJenkinson you changed a prop this is a Breaking Change but you did not check the breaking change option 🤔

@BenJenkinson
Copy link
Contributor Author

you changed a prop this is a Breaking Change but you did not check the breaking change option 🤔

@crathor, you're right. I have now checked that checkbox.

@davidacevedo
Copy link
Collaborator

davidacevedo commented May 19, 2022

Can you instead update the types to be direction so that intellisense is happy https://github.com/reactstrap/reactstrap/blob/master/types/lib/Offcanvas.d.ts. I'd rather us not make a breaking change for something small like this yet as direction is already an available prop that works while placement "doesn't exist"

@BenJenkinson
Copy link
Contributor Author

BenJenkinson commented May 19, 2022

Because we have both direction and placement, there's going to be some sort of breaking change either way.

Either JS users need to switch from direction to placement, or TS users have to switch from (valid, but non-working) placement to direction.

Ideally, I would prefer placement, as that would match the Bootstrap documentation, and match the other Tooltip/Popover components that have a placement prop.

But, I suppose we should prefer forcing a breaking change on TS users rather than JS users.

  • It would only affect TS users who have tried to use the (non-working) placement prop.
  • If it were the other way around, then any JS users who had used direction would silently find that their <Offcanvas/> element switched direction.

@BenJenkinson BenJenkinson changed the title fix(Offcanvas): use placement prop rather than direction (#2511) fix(Offcanvas): make types match existing direction prop May 19, 2022
@BenJenkinson
Copy link
Contributor Author

Hi @davidacevedo,

Are you OK with these changes?

Copy link
Collaborator

@davidacevedo davidacevedo left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@davidacevedo davidacevedo merged commit 13cbeeb into reactstrap:master May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offcanvas has incorrect prop types for placement/direction
3 participants