-
Notifications
You must be signed in to change notification settings - Fork 120
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
feat: adding toggle text as an option for accordion component #1537
base: main
Are you sure you want to change the base?
Conversation
68e9b80
to
94be876
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1537 +/- ##
==========================================
+ Coverage 93.74% 93.84% +0.10%
==========================================
Files 154 154
Lines 11391 11408 +17
Branches 701 704 +3
==========================================
+ Hits 10678 10706 +28
+ Misses 692 680 -12
- Partials 21 22 +1 ☔ View full report in Codecov by Sentry. |
94be876
to
bbf2cc8
Compare
<Text.span textStyle='captionBold' color='primary.base' data-text-toggle='open'> | ||
Less | ||
</Text.span> | ||
<Text.span textStyle='captionBold' color='primary.base' data-text-toggle='closed'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the whole header is interactive, i don't think you need to include this 'data-text-toggle' stuff, it should just work anyway shouldn't it?
can you not just use the existing
&[data-state='closed/open'] { }
state in the css and target a class on this span, rather than having now 2 state for open/close?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'm using [data-state='closed/open']
to check if the accordion is open or closed, I added data-text-toggle
just to keep track of which span should I show More/Less
, but I can change it for a class, no problem
bbf2cc8
to
333e691
Compare
Accordion: Added
useToggleText
prop to allow customization of the toggle element. When set totrue
, the default arrow will be replaced with "More" and "Less" textAccordion.Trigger: Fixed a bug where the
hoverBg
prop was not being applied correctly