fix(accordion): update styles for ionic theme#31023
fix(accordion): update styles for ionic theme#31023OS-susmitabhowmik wants to merge 26 commits intonextfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
OS-susmitabhowmik
added
type: bug
OS-susmitabhowmik
changed the title
| // Item in Accordion | ||
| // -------------------------------------------------- | ||
| // Ensure button element in accordion header has proper horizontal padding specifically for nested accordions | ||
| :host(.accordion-header-item) .item-native { | ||
| @include globals.padding-horizontal(globals.$ion-space-400); | ||
| } |
There was a problem hiding this comment.
This style doesn't seem to be applied. I commend this out locally and the tests still passed. After inspecting the element, this is already being applied through the slot styles.
Regardless, any padding changes must be done through the slot styles that I linked.
| // Item in Accordion | |
| // -------------------------------------------------- | |
| // Ensure button element in accordion header has proper horizontal padding specifically for nested accordions | |
| :host(.accordion-header-item) .item-native { | |
| @include globals.padding-horizontal(globals.$ion-space-400); | |
| } |
There was a problem hiding this comment.
It's applied within nested accordions so that the padding of the child accordions match the padding for the parent. The slot styles don't seem to work as expected, I think perhaps because the nested accordion items are within the shadow DOM. Removing these styles and class removes the horizontal padding for the nested items leading to inconsistencies in the focus styles between the parent and children. I also don't think we previously had tests for nested accordions with the ionic theme, I included them as a part of this commit a3a76f1.
There was a problem hiding this comment.
Since the goal is to line up the nested accordion header items, then we can simply update the following code to:
ion-accordion > [slot="content"] ion-item {
@include globals.typography(globals.$ion-body-md-regular);
color: globals.$ion-primitives-neutral-1000;
- --padding-start: 0;
--padding-top: #{globals.$ion-space-300};
--padding-bottom: #{globals.$ion-space-300};
- --padding-end: 0;
--min-height: #{globals.$ion-scale-700};
}
+ ion-accordion > [slot="content"] ion-item:not([slot="header"]) {
+ --padding-start: 0;
+ --padding-end: 0;
+ }I was able to get the same result, I verified by running tests locally and they all pass.
Make sure to restart the server if you're running it else it doesn't apply the changes within the core styles.
|
|
||
| /** | ||
| * Add a class to identify this item as an accordion header | ||
| * for specific styling purposes | ||
| */ | ||
| ionItem.classList.add('accordion-header-item'); |
There was a problem hiding this comment.
This can be removed based on my comment.
| /** | |
| * Add a class to identify this item as an accordion header | |
| * for specific styling purposes | |
| */ | |
| ionItem.classList.add('accordion-header-item'); |
There was a problem hiding this comment.
Unfortunately I don't think we can remove this class as it breaks the styling for nested accordions. Please see my comment here but I am happy to try any alternative suggestions!
There was a problem hiding this comment.
These changes can be reverted based on my comment.
There was a problem hiding this comment.
I replied above (#31023 (comment)), but I think we may need to keep the new class.
|
@OS-susmitabhowmik Also please update the PR title to be shorter, thanks! |
OS-susmitabhowmik
force-pushed
the
ROU-11979-fix-accordian-styles
branch
from
807b6dc to
f6ecd8f
Compare
OS-susmitabhowmik
changed the title
@thetaPC Updated! |
3 participants
Issue number: resolves internal
What is the current behavior?
There were several issues in the ionic theme which are resolved in this PR:
What is the new behavior?
The following changes were made in the ionic theme:
Does this introduce a breaking change?
Other information