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

fix(accordion-item): split header content padding #10865

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

alisonailea
Copy link
Contributor

Related Issue: #10762

Summary

Splits header and content padding rules so the token doesn't apply to header

@benelan
Copy link
Member

benelan commented Dec 6, 2024

Do we have any conventions for what constitutes a breaking change to the design tokens? Because changes to the json paths could break someone's build script if they are accessing token values for use in their application.

@alisonailea alisonailea force-pushed the astump/10762-accordion-item-header-content-padding branch from 7b705c7 to 32e76be Compare December 9, 2024 23:43
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Dec 9, 2024
@alisonailea
Copy link
Contributor Author

Do we have any conventions for what constitutes a breaking change to the design tokens? Because changes to the json paths could break someone's build script if they are accessing token values for use in their application.

This PR should not have included the space and size token updates which were covered in a separate PR.

@benelan benelan added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Dec 12, 2024
@benelan
Copy link
Member

benelan commented Dec 12, 2024

Do we have any conventions for what constitutes a breaking change to the design tokens? Because changes to the json paths could break someone's build script if they are accessing token values for use in their application.

This PR should not have included the space and size token updates which were covered in a separate PR.

Gotcha was that #10727?

Do you think we should add snapshot or test coverage for this fix?

Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should add snapshot or test coverage for this fix?

Nevermind, it looks like the existing Custom Theme story picked up this change. Code LGTM, but I'll defer to design for the chromatic approval. cc @ashetland

Copy link

@ashetland ashetland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alisonailea alisonailea merged commit e9e307c into dev Dec 12, 2024
16 checks passed
@alisonailea alisonailea deleted the astump/10762-accordion-item-header-content-padding branch December 12, 2024 17:33
@github-actions github-actions bot added this to the 2024-12-17 - Dec Milestone milestone Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants