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

feat(Accordion): add new filled variant #2065

Merged
merged 50 commits into from
Mar 14, 2024

Conversation

saurabhdaware
Copy link
Member

@saurabhdaware saurabhdaware commented Mar 5, 2024

Description

Modify accordion

Changes

  • Add size, variant prop
  • Add AccordionItemHeader, AccordionItemBody components
  • Add deprecation handling to support old API
  • Add tests for new API and keep some deprecation tests to ensure old API doesn't break
  • Update documentation

For API changes, check out - #2058

Additional Information

TODO

  • Finalize variant="filled" | "transparent" vs variant="bordered" | "borderless"
  • Release on Figma

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

Copy link

changeset-bot bot commented Mar 5, 2024

🦋 Changeset detected

Latest commit: efa8ca9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@razorpay/blade Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Mar 5, 2024

✅ PR title follows Conventional Commits specification.

@saurabhdaware saurabhdaware marked this pull request as ready for review March 8, 2024 08:07
anuraghazra
anuraghazra previously approved these changes Mar 12, 2024
Copy link
Member

@anuraghazra anuraghazra left a comment

Choose a reason for hiding this comment

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

:shipit:

@saurabhdaware saurabhdaware added Review - L2 Second level of review and removed Review - L1 First level of review labels Mar 13, 2024
const isExpanded = expandedIndex === _index;
const isDefaultExpanded = defaultExpandedIndex === _index;
const isDeprecatedAPI = Boolean(title) || Boolean(description);
const [header, body] = React.Children.toArray(children);
Copy link
Collaborator

Choose a reason for hiding this comment

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

where are we throwing an error when there's something else apart from header and body?

Copy link
Member Author

Choose a reason for hiding this comment

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

we can't until we remove deprecated API. Because passing anything is valid in the old API. People can pass any child and that becomes the content slot for old API.

<AccordionItem>
  <div>Random content</div>
</AccordionItem>

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh right! the checks you have added should be sufficient 👍

<AccordionButton
index={_index}
icon={icon}
title={title}
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be _title like we have _description? or since AccordionButton is not exposed to the consumers hence this is kept this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup AccordionButton is not exposed so the component itself is internal unlike AccordionItemBody which is exposed and where description is not a valid prop from consumer end. We just have to internally maintain it to support old API.

* it sometimes messes up the layout calculation (it thinks of multiline as a single line before flowing in the UI).
* And during the expanding / collapsing animation, text reflows causing words to jump around across lines.
*
* Rendering a blank `Text` at the end seems to fix all this 🤯
Copy link
Collaborator

Choose a reason for hiding this comment

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

what sorcery is this ? 😮

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know 😆 Credits to Divyanshu

@@ -56,4 +58,8 @@ const AccordionItemHeader = ({
);
};

const AccordionItemHeader = assignWithoutSideEffects(_AccordionItemHeader, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we still need this assignWithoutSideEffects? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't test after the bundling changes we might not need it 🤔

Base automatically changed from accordion/api-changes to master March 13, 2024 12:38
@saurabhdaware saurabhdaware dismissed stale reviews from kamleshchandnani and anuraghazra March 13, 2024 12:38

The base branch was changed.


We have added `AccordionItemHeader` and `AccordionItemBody` components.

Props like `icon` and `title` move from AccordionItem to AccordionItemHeader and `description` moves to AccordionItemBody.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Props like `icon` and `title` move from AccordionItem to AccordionItemHeader and `description` moves to AccordionItemBody.
Props like `icon` and `title` on AccordionItem are deprecated and moved to AccordionItemHeader and `description` moves to AccordionItemBody.

Copy link
Member Author

Choose a reason for hiding this comment

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

reworded it a bit because they dont just move. E.g. icon becomes leading with different syntax, description is passed as children

@saurabhdaware saurabhdaware merged commit 524fa92 into master Mar 14, 2024
16 checks passed
@saurabhdaware saurabhdaware deleted the accordion/new-improvements branch March 14, 2024 09:39
anuraghazra pushed a commit that referenced this pull request Apr 9, 2024
* feat: add new accordion props

* feat: update Accordion API

* feat: add alternate api

* feat: add bordered variant

* feat: add baseheader to accordion

* feat: allow multiple items

* fix: remove border gap

* feat: add size in context

* feat: add new API

* feat: update for new changes

* feat: add open question

* feat: replace JSX with ReactNode

* feat: add size prop on BaseHeader

* feat: adjust centerbox for baseheader size

* feat: keep old description API working

* feat: add size to deprecated icon

* feat: add jsdoc for accordion

* fix: showNumberPrefix prop

* fix: react-native-reanimated bug

* fix: Accordion for React Native

* feat: update docs example

* fix: example api

* tests: update snapshots

* fix: weird ts error

* feat: add stopPropagation to link

* fix: baseheader test

* feat: align chevron correctly

* fix: icon alignment in react native

* fix: snapshots

* revert rn sb change

* feat: update to solid | transparent variant naming

* feat: rename variant values

* fix: ts

* fix: remove isExpanded from header

* fix: add disabled prop to BaseHeader

* fix: add disabled item

* feat: add disabled to accordionitem

* feat: handle cursor on web

* resolve anurag's comments

* docs: add real world examples

* feat: add validation

* feat: add metaAttributes

* feat: update variant value names

* Create late-planes-destroy.md

* docs: reword migration list

* remove code tag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - L2 Second level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants