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: drag to re-arrange custom columns not working #1299

Merged
merged 3 commits into from
May 16, 2023
Merged

Conversation

dsmmcken
Copy link
Contributor

@dsmmcken dsmmcken commented May 15, 2023

resolves: #1282

Broken by #1013. Can't use prop spreading with our Button component, added support for spreading.
Needs silverheels cherry-pick.

resolves: #1282

Broken by #1013. Can't use prop spreading with our Button component.
Revert to previous button. Needs silverheels cherry-pick.
@dsmmcken dsmmcken self-assigned this May 15, 2023
@dsmmcken dsmmcken requested a review from mattrunyon May 15, 2023 19:50
@mattrunyon
Copy link
Collaborator

Any reason to not allow prop spreading any unknown props to our button? There's probably just some specific event handler we don't explicitly add to our button that react beautiful dnd uses

@dsmmcken
Copy link
Contributor Author

I thought in general it was frowned upon, given our eslint rule? I have no preference for how it should be fixed.

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #1299 (77644ca) into main (a80c6fc) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1299   +/-   ##
=======================================
  Coverage   45.88%   45.89%           
=======================================
  Files         492      492           
  Lines       34362    34371    +9     
  Branches     8567     8566    -1     
=======================================
+ Hits        15768    15775    +7     
- Misses      18543    18545    +2     
  Partials       51       51           
Flag Coverage Δ
unit 45.89% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/components/src/Button.tsx 76.31% <ø> (ø)

... and 30 files with indirect coverage changes

@mattrunyon
Copy link
Collaborator

This is one of the exceptions to the rule I think. The button component has a bunch of extra HTML props that it can/should be able to accept (it should be a superset of the base button). So it would be silly to explicitly add every prop instead of spreading those we don't handle in some specific way

@dsmmcken dsmmcken merged commit 5e23e4a into main May 16, 2023
@dsmmcken dsmmcken deleted the dmckenzie_1282 branch May 16, 2023 18:07
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drag to re-arrange custom columns not working
2 participants