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.

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

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

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

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

fix #1269

image

@stackblitz StackBlitz
Copy link

stackblitz bot commented Feb 15, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@vercel Vercel
Copy link

vercel bot commented Feb 15, 2024 •
edited

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 Codecov
Copy link

codecov bot commented Feb 15, 2024 •
edited

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

@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

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
@dhavalveera
Copy link
Contributor Author

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

@SutuSebastian - this codecov issue again

@SutuSebastian SutuSebastian merged commit d0dc810 into themesberg:main Mar 12, 2024
4 of 5 checks passed
chunkerchunker added a commit to chunkerchunker/flowbite-react that referenced this pull request Mar 28, 2024
@chunkerchunker
Fix React warning in ButtonGroup
544cb84 
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)
@chunkerchunker chunkerchunker mentioned this pull request Mar 28, 2024
Open
@chunkerchunker
Copy link

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

SutuSebastian pushed a commit to chunkerchunker/flowbite-react that referenced this pull request Mar 29, 2024
@chunkerchunker @SutuSebastian
Fix React warning in ButtonGroup
1ba2a8f 
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)
@github-actions github-actions bot mentioned this pull request Mar 29, 2024
Merged
SutuSebastian pushed a commit to chunkerchunker/flowbite-react that referenced this pull request Mar 29, 2024
@chunkerchunker @SutuSebastian
Fix React warning in ButtonGroup
ca4f841 
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)
This was referenced May 21, 2024
Open
Open
@Abellache-Rabah Abellache-Rabah mentioned this pull request May 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SutuSebastian SutuSebastian SutuSebastian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dynamically generated buttons within a button group are not properly styled.
3 participants
@dhavalveera @chunkerchunker @SutuSebastian