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

Improve sidebar accordion #409

Open
wants to merge 7 commits into
base: XP-3053-search-profile-display
Choose a base branch
from

Conversation

Moharu
Copy link
Contributor

@Moharu Moharu commented Jul 4, 2019

Release Type: Dev Improvements

Creates the component mentioned in https://github.com/x-team/xp/issues/1927

Description

Creates a dedicated Accordion UI Component that holds all knowledge about its GenericCollapsible children, abstracting all necessary toggle controls.

Checklist

Before submitting a pull request, please make sure the following is done:

  • set yourself as an assignee
  • set appropriate labels for a PR (In Review or In Progress depending on its status)
  • make sure your code lints (npm run lint)
  • Flow typechecks passed (npm run flow)
  • Snapshots tests passed (npm run jest)
  • component's documentation (.stories.js file) is changed or added accordingly to reflect any new or updated use cases or variants usage
  • if any snapshots have been changed, verify that component still works and looks as expected and update the changed snapshot

Steps to Test or Reproduce

Check the multiple cases for GenericCollapsibleAccordion in Storybook.

Impacted Areas in Application

GenericCollapsibleAccordion

@Moharu Moharu self-assigned this Jul 4, 2019
@Moharu Moharu marked this pull request as ready for review July 4, 2019 16:05
Copy link
Contributor

@bernardodiasc bernardodiasc left a comment

Choose a reason for hiding this comment

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

This looks very good! I tested locally on Storybook and works as expected! Well done @Moharu 🎉

Just left a suggestion to change let for const. Unless I missed the reason of that choice. Everything else looks great to me.


componentDidMount = () => {
if (!this.state.isControlled) {
let { children } = this.props
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be const instead?

componentDidMount = () => {
if (!this.state.isControlled) {
let { children } = this.props
let childrenCount = React.Children.count(children)
Copy link
Contributor

Choose a reason for hiding this comment

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

This also can be const

@nicksp nicksp changed the title GH-1927 improve sidebar accordion Improve sidebar accordion Apr 9, 2020
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