Skip to content

Add an unset option to the collapse active key - #180

Open
itssulaimann wants to merge 3 commits into
react-component:masterfrom
itssulaimann:patch-1
Open

Add an unset option to the collapse active key#180
itssulaimann wants to merge 3 commits into
react-component:masterfrom
itssulaimann:patch-1

Conversation

@itssulaimann

Copy link
Copy Markdown

Having an unset option to provide easier customization to the collapse

Having an unset option to provide easier customization to the collapse
@vercel

vercelBot commented Nov 30, 2020

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/collapse/6qe4in9iw
✅ Preview: https://collapse-git-patch-1.react-component.vercel.app

@codecov

codecovBot commented Nov 30, 2020

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.13%. Comparing base (ea1ab91) to head (94b6195).
⚠️ Report is 106 commits behind head on master.

Files with missing linesPatch %Lines
src/Collapse.tsx66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## master #180 +/- ##
==========================================
- Coverage 99.04% 98.13% -0.92% 
==========================================
Files 4 4 Lines 105 107 +2 Branches 33 34 +1 ==========================================
+ Hits 104 105 +1 - Misses 1 2 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Update the ternary operator to if statement.
@itssulaimann

Copy link
Copy Markdown
Author

Having an unset option to the activeKey prop in order to make the component easier to handle

@lgtm-com

lgtm-comBot commented Nov 30, 2020

Copy link
Copy Markdown
Contributor

This pull request introduces 12 alerts when merging 06c92a3 into ea1ab91 - view on LGTM.com

new alerts:

  • 12 for Syntax error

@itssulaimannitssulaimann left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Update the condition of the if statement in Collapse component

@nrps9909

Copy link
Copy Markdown

I revalidated this request against current master dcd0f78. The requested “unset” behavior already has a supported representation: activeKey={undefined} is treated as uncontrolled by useControlledState, so defaultActiveKey is used and subsequent clicks update the internal state.

A current-master regression matrix confirmed both boundaries: activeKey={undefined} + defaultActiveKey="default" rendered the default panel active and allowed a second panel to open, while activeKey="unset" correctly activated a panel whose literal key is "unset". The full file passed 97/97 tests and one snapshot with those temporary cases.

Porting this PR’s sentinel semantics to the current hook kept the undefined case passing but made the literal-key regression fail with zero active panels. That is an API collision because activeKey and panel keys publicly accept arbitrary React.Key strings; "unset" cannot safely be reserved as an out-of-band control value.

This exact head 94b61956692914cc3edb26d89d9fede5325fcada is also over 100 commits behind current master and has no regression coverage. I recommend closing it rather than rebasing. Consumers can pass undefined for the existing uncontrolled fallback; if a distinct reset command is still desired, it needs an explicit API outside the panel-key namespace.

AI assistance disclosure: Codex was used to trace the current controlled-state contract, run the temporary regression matrix, and draft this note. I verified the cited heads and restored the worktree clean before posting.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@itssulaimann@nrps9909