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

Remove defaults from button theme & update border merge #185

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

dickermoshe
Copy link
Contributor

Closes #184

@nank1ro
Copy link
Owner

nank1ro commented Nov 2, 2024

There is something wrong with your PR.
I ran the example, opened the Accordion page and it throws:

══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following assertion was thrown building ShadDecorator(dirty, dependencies:
[ShadInheritedTheme]):
'package:shadcn_ui/src/theme/components/decorator.dart': Failed assertion: line 138 pos 12:
'BorderSide.canMerge(top, other.top)': is not true.

The relevant error-causing widget was:
  ShadDecorator
  ShadDecorator:file:///Users/ale/github/flutter-shadcn-ui/lib/src/components/switch.dart:174:22

When the exception was thrown, this was the stack:
#2      ShadBorder.mergeWith (package:shadcn_ui/src/theme/components/decorator.dart:138:12)
#3      ShadDecoration.mergeWith (package:shadcn_ui/src/theme/components/decorator.dart:303:35)

@nank1ro
Copy link
Owner

nank1ro commented Nov 2, 2024

Are you sure the BorderSide.canMerge is needed?
Seems like something the Flutter team decided but I don't understand the reason.

/// Two sides can be merged if one or both are zero-width with
/// [BorderStyle.none], or if they both have the same color and style.

I want shadcn_ui to allow every kind of customizations, and not limit for the same style or color.

@dickermoshe
Copy link
Contributor Author

Forgive me, I misunderstood what BorderSide.merge does. Turns out it's a pretty badly named function.
That function adds the 2 borders, instead of merging them.

Turns out you can't merge Borders, so defining one completely overwrites the previous one, even if you just want to change the color.

Gonna remove the changes and just keep the button stuff. Sounds Good?

@nank1ro
Copy link
Owner

nank1ro commented Nov 3, 2024

Sounds good 👍

@dickermoshe
Copy link
Contributor Author

done.

@nank1ro nank1ro merged commit 356ef04 into nank1ro:main Nov 4, 2024
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.

ShadBorder sides isn't merged
2 participants