Skip to content

Resolve inconsistent behavior in 'show' and 'hide' methods#41301

Closed
D07N37 wants to merge 1 commit intotwbs:mainfrom
D07N37:fix-show-and-hide-behavior
Closed

Resolve inconsistent behavior in 'show' and 'hide' methods#41301
D07N37 wants to merge 1 commit intotwbs:mainfrom
D07N37:fix-show-and-hide-behavior

Conversation

@D07N37
Copy link
Copy Markdown

@D07N37 D07N37 commented Mar 15, 2025

Description

Currently the _isShown property is being checked in the 'show' and 'hide' methods, but the _isShown property is never changed, causing it to stay false no matter the component's state. This behavior is inconsistent with the behavior of other components, and can cause undesired behavior when the same method is called multiple times in succession.

This PR introduces code in the constructor to set the _isShown property based on the current classes of the component (in accordance with the documentation), and to update the _isShown property in the 'show' and 'hide' methods.

Motivation & Context

Currently the 'show' and 'hide' methods of the collapse component act like its 'toggle' method, ignoring whether the component is actually shown or hidden.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Ensure that the 'show' method will only cause the element to be shown, and that the 'hide' method will only cause the element to be hidden, regardless of the current state
@D07N37 D07N37 requested a review from a team as a code owner March 15, 2025 04:28
@julien-deramond
Copy link
Copy Markdown
Member

Thanks for your contribution, @D07N37! We haven't reviewed the content yet, but could you please add some unit tests to this PR? Ideally, they should fail on the main branch and pass with your changes.

@D07N37
Copy link
Copy Markdown
Author

D07N37 commented Mar 15, 2025

Looking through the unit tests, this._isShown is actually a method so the original edits do not work

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants