Skip to content

Conversation

@rafeca
Copy link
Contributor

@rafeca rafeca commented Sep 27, 2019

#499 introduced what appears to be either a bug or an unnoticed/undocumented breaking change:

With #499, the Details component instead of using an open property to open/show the contents on initial mount, it now uses a defaultOpen property.

I've assumed that this is a bug instead of a breaking change, since there's no mention of this on the breaking changes list. Also the docs, the Typescript definitions and the tests are referring to open instead of defaultOpen (in fact I could reenable that last test thanks to this change).

Alternatively, if we want to stick to defaultOpen (which to be honest I think it's a better property name), I can change this PR to update docs/tests/TS types. Let me know your preference 😃

Merge checklist

  • Changed base branch to release branch
  • Add or update TypeScript definitions (index.d.ts) if necessary
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@vercel
Copy link

vercel bot commented Sep 27, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://primer-components-git-fork-rafeca-fix-details-open-property.primer.now.sh

@vercel vercel bot temporarily deployed to staging September 27, 2019 11:54 Inactive
@jonrohan jonrohan requested a review from emplums September 30, 2019 16:23
@emplums
Copy link

emplums commented Sep 30, 2019

Hey @rafeca sorry for the bad documentation around this! The defaultOpen is just used to set the starting state for the Details component - the open prop is still around as a render prop so that consumers can have access to whether or not the component is open if they'd like to conditionally manipulate the UI depending on that state, but consumers shouldn't manually handle the state of open. I'll update the documentation accordingly!

@emplums emplums closed this Sep 30, 2019
@emplums emplums mentioned this pull request Sep 30, 2019
Sign up for free to 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