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

feat(accordion): change buttonWidth prop type to string (FE-5709) #5914

Merged
merged 3 commits into from
Mar 24, 2023
Merged

feat(accordion): change buttonWidth prop type to string (FE-5709) #5914

merged 3 commits into from
Mar 24, 2023

Conversation

grabkowski
Copy link
Contributor

@grabkowski grabkowski commented Mar 13, 2023 •
edited
Loading

addresses #5897

Proposed behaviour

buttonWidth prop should accept all valid string CSS width values

Current behaviour

buttonWidth prop accepts only numbers and a "px" string is added in CSS

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
    - [ ] Screenshots are included in the PR if useful
    - [ ] All themes are supported if required
    - [ ] Unit tests added or updated if required
    - [ ] Cypress automation tests added or updated if required
    - [ ] Storybook added or updated if required
    - [ ] Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
    - [ ] Related docs have been updated if required

QA

  • Tested in CodeSandbox/storybook
  • Add new Cypress test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by codesandbox[bot].

@grabkowski grabkowski self-assigned this Mar 13, 2023
@cypress
Copy link

cypress bot commented Mar 13, 2023 •
edited
Loading

Passing run #15559 ↗︎

0 4785 98 0 Flakiness 0

Details:

Merge branch 'master' into FE-5709-accordion-modify-buttonwidth-type-to-string
Project: carbon Commit: 99603893d8
Status: Passed Duration: 05:21 💡
Started: Mar 24, 2023 11:35 AM Ended: Mar 24, 2023 11:40 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@grabkowski grabkowski force-pushed the FE-5709-accordion-modify-buttonwidth-type-to-string branch 3 times, most recently from f30f1e5 to faa3486 Compare March 13, 2023 10:41
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 13, 2023 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9960389:

Sandbox Source
carbon-quickstart Configuration
carbon-quickstart-typescript Configuration

@grabkowski grabkowski force-pushed the FE-5709-accordion-modify-buttonwidth-type-to-string branch from faa3486 to 046dbf3 Compare March 13, 2023 11:23
edleeks87
edleeks87 reviewed Mar 13, 2023
View reviewed changes
Copy link
Contributor

@edleeks87 edleeks87 left a comment

Choose a reason for hiding this comment

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

Overall the proposed changes look good to me, just the one question around whether we should avoid the breaking changes

src/components/accordion/accordion.component.tsx Outdated
@@ -21,7 +21,7 @@ export interface AccordionProps
extends StyledAccordionContainerProps,
SpaceProps {
/** Width of the buttonHeading when it's set, defaults to 150px */
buttonWidth?: number;
buttonWidth?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should avoid the breaking change here and support both number and string. If it's a number value then we can keep the current behaviour of appending px to it. If it's a string we just apply the value and let the implementing developers set the unit etc. What do you think?

@Parsium Parsium self-requested a review March 20, 2023 11:20
Parsium
Parsium reviewed Mar 20, 2023
View reviewed changes
Copy link
Contributor

@Parsium Parsium left a comment

Choose a reason for hiding this comment

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

Just one minor comment from me, happy to approve though once @edleeks87's comment has been addressed 😄

src/components/accordion/accordion.test.js Outdated Show resolved Hide resolved
@grabkowski grabkowski force-pushed the FE-5709-accordion-modify-buttonwidth-type-to-string branch 2 times, most recently from 4281fbe to f48a2ec Compare March 20, 2023 14:38
@grabkowski grabkowski force-pushed the FE-5709-accordion-modify-buttonwidth-type-to-string branch from f48a2ec to 3aa5b2f Compare March 21, 2023 12:02
@grabkowski grabkowski force-pushed the FE-5709-accordion-modify-buttonwidth-type-to-string branch from 3aa5b2f to 34aa10c Compare March 21, 2023 12:05
@Parsium Parsium marked this pull request as ready for review March 22, 2023 10:22
@Parsium Parsium requested review from a team as code owners March 22, 2023 10:22
@grabkowski grabkowski merged commit 1f4e00c into master Mar 24, 2023
@grabkowski grabkowski deleted the FE-5709-accordion-modify-buttonwidth-type-to-string branch March 24, 2023 13:23
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 116.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZhuoyuJin ZhuoyuJin ZhuoyuJin approved these changes

@edleeks87 edleeks87 edleeks87 approved these changes

@Parsium Parsium Parsium approved these changes

Assignees

@grabkowski grabkowski

Labels
No UX QA Required Passed QA Passed Review released Squad Front End
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grabkowski @carbonci @Parsium @edleeks87 @ZhuoyuJin