Skip to content

fix(ButtonGroup): dynamic generated button with group wasn't styled properly #1273

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 12, 2024
Merged

fix(ButtonGroup): dynamic generated button with group wasn't styled properly #1273

merged 1 commit into from
Mar 12, 2024

Conversation

@dhavalveera
Copy link
Contributor

@dhavalveera dhavalveera commented Feb 15, 2024

Dynamically generated buttons within a button group are not properly styled.

fix #1269

image
@bolt-new-by-stackblitz
Copy link

bolt-new-by-stackblitz bot commented Feb 15, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.
@vercel
Copy link

vercel bot commented Feb 15, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flowbite-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 0:39am
@codecov
Copy link

codecov bot commented Feb 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.85714% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 97.29%. Comparing base (7461173) to head (9d328a5).
Report is 197 commits behind head on main.

Files Patch % Lines
src/components/Button/ButtonGroup.tsx 82.85% 6 Missing ⚠️
Additional details and impacted files 
@@ Coverage Diff @@ ## main #1273 +/- ## ========================================== - Coverage 99.54% 97.29% -2.26%  ========================================== Files 163 216 +53 Lines 6621 9245 +2624 Branches 401 542 +141 ========================================== + Hits 6591 8995 +2404  - Misses 30 250 +220

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.
@dhavalveera
Copy link
Contributor Author

dhavalveera commented Feb 15, 2024 •
edited
Loading

@SutuSebastian - please have a look & review the fix of the bug mentioned in Issue #1269 .
@SutuSebastian SutuSebastian changed the title fix(ButtonGroup.tsx): dynamic generated button with group wasnt styled properly fix(ButtonGroup): dynamic generated button with group wasn't styled properly Feb 15, 2024
@dhavalveera
Copy link
Contributor Author

dhavalveera commented Mar 10, 2024

Hello @SutuSebastian , Sorry to bother you, but can you please review this PR?
SutuSebastian
SutuSebastian approved these changes Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@SutuSebastian SutuSebastian 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 a quick fix, we need to find a way to target the Button component while recursively searching through children, and only inject props into the button, not all children components.

let's merge this until that "final" solution appears
@dhavalveera @SutuSebastian
fix(/button/buttongroup.tsx): dynamic generated buton with group wasn…
9d328a5 
…t styled properly Dynamically generated buttons within a button group are not properly styled. themesberg#1269 fix themesberg#1269
@SutuSebastian SutuSebastian force-pushed the fix/button-group-dynamic branch from 33ae831 to 9d328a5 Compare March 12, 2024 12:35
@dhavalveera
Copy link
Contributor Author

dhavalveera commented Mar 12, 2024

this is a quick fix, we need to find a way to target the Button component while recursively searching through children, and only inject props into the button, not all children components.

let's merge this until that "final" solution appears

Yeah!. Let's find a proper solution for it
@dhavalveera
Copy link
Contributor Author

dhavalveera commented Mar 12, 2024

@SutuSebastian - this codecov issue again
@SutuSebastian SutuSebastian merged commit d0dc810 into themesberg:main Mar 12, 2024
@chunkerchunker chunkerchunker mentioned this pull request Mar 28, 2024
Merged
@chunkerchunker
Copy link
Contributor

chunkerchunker commented Mar 28, 2024

this is a quick fix, we need to find a way to target the Button component while recursively searching through children, and only inject props into the button, not all children components.
let's merge this until that "final" solution appears

Yeah!. Let's find a proper solution for it

Suggested fix in #1323
@github-actions github-actions bot mentioned this pull request Mar 29, 2024
Merged
This was referenced May 21, 2024
Open
Open
@Abellache-Rabah Abellache-Rabah mentioned this pull request May 24, 2024
Open
@tangentdigitalagency tangentdigitalagency mentioned this pull request Jun 6, 2024
Open
@joezilla joezilla mentioned this pull request Jun 7, 2024
Open
SutuSebastian pushed a commit that referenced this pull request Jun 19, 2024
@chunkerchunker
Fix React warning in ButtonGroup (#1323)
50c860e 
* Fix React warning in ButtonGroup This change fixes the following issue: ButtonGroup was adding positionInGroup to all children rather than just Buttons. For example, in the following, both the Buttons and the child spans get positionInGroup props: ``` <Button.Group><Button><span>b1</span></Button><Button><span>b2</span></Button></Button.Group> ``` This results in a React warning: “Warning: React does not recognize the `positionInGroup` prop on a DOM element...” The review comment in the original code change causing this hints at this issue as well: #1273 (comment) * fix formatting
jhlee-gsenergy pushed a commit to gswindpower/flowbite-react that referenced this pull request Feb 6, 2025
@chunkerchunker
Fix React warning in ButtonGroup (themesberg#1323)
88e6d3e 
* Fix React warning in ButtonGroup This change fixes the following issue: ButtonGroup was adding positionInGroup to all children rather than just Buttons. For example, in the following, both the Buttons and the child spans get positionInGroup props: ``` <Button.Group><Button><span>b1</span></Button><Button><span>b2</span></Button></Button.Group> ``` This results in a React warning: “Warning: React does not recognize the `positionInGroup` prop on a DOM element...” The review comment in the original code change causing this hints at this issue as well: themesberg#1273 (comment) * fix formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@dhavalveera @chunkerchunker @SutuSebastian