Skip to content

Conversation

@bhennes2
Copy link

Author Checklist

  • Add unit test(s)
  • Update version in package.json (see the versioning guidelines)
  • Update documentation (if necessary)
  • Add story to storybook (if necessary)
  • Assign dev reviewer

@bhennes2
Copy link
Author

@chawes13 Also looking to port over some default styles for this component too from MTC. Ben is working to parse out those styles that I'd like to include in this PR.

- `input` **[Object][156]** A `redux-forms` [input][157] object
- `meta` **[Object][156]** A `redux-forms` [meta][158] object
- `options` **[Array][154]** An array of checkbox values (strings or key-value pairs)
- `selectedOptionsDisplayFormatter` **[Function][]** A function that receives an array of selected values and returns a string or HTML to render those values
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it return HTML or JSX?

<div className="dropdown-select">
<div className="select-input" onClick={ toggleExpanded }>
<p>{ getLabel(selectedValues) }</p>
<p>{selectedOptionsDisplayFormatter(selectedValues)}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this has the possibility to return jsx, should we make it a component (a la LabeledField's labelComponent)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I typically would interpret a "Formatter" as something that returns a string, but I might just be biased by my experience with date libraries (e.g., date-fns and moment)

expect(newValue).toEqual([])
})

test('DropdownCheckboxGroup allows custom selected option formatting', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

A test for jsx would be nice as well

@chawes13
Copy link
Contributor

@bhennes2 Has your local implementation held up on your client project? Still interested in including this functionality in the library?

@josiasds
Copy link
Contributor

@CodiumAI-Agent /improve --extended

@CodiumAI-Agent
Copy link

PR Code Suggestions

💡 Suggestion:

Correct the validator's logic by passing the selectedOptionsDisplayFormatter prop to the DropdownSelect component.

File: src/forms/inputs/dropdown-checkbox-group.js (62-64)

Example code:

Existing code:

<DropdownSelect {...props} selectedValues={ value } className="checkboxes">
  <CheckboxGroup { ...props } label={ false } />
</DropdownSelect>

Improved code:

<DropdownSelect {...props} selectedValues={ value } selectedOptionsDisplayFormatter={props.selectedOptionsDisplayFormatter} className="checkboxes">
  <CheckboxGroup { ...props } label={ false } />
</DropdownSelect>

💡 Suggestion:

Wrap the PropTypes and defaultProps into a function to improve reusability and maintainability.

File: src/forms/helpers/dropdown-select.js (7-18)

Example code:

Existing code:

const propTypes = {
  children: PropTypes.node,
  className: PropTypes.string,
  selectedValues: PropTypes.arrayOf(PropTypes.string),
  selectedOptionsDisplayFormatter: PropTypes.func,
  ...togglePropTypes('expanded'),
}

const defaultProps = {
  className: '',
  selectedValues: [],
  selectedOptionsDisplayFormatter: getLabel,
}

Improved code:

function getDropdownSelectProps() {
  return {
    propTypes: {
      children: PropTypes.node,
      className: PropTypes.string,
      selectedValues: PropTypes.arrayOf(PropTypes.string),
      selectedOptionsDisplayFormatter: PropTypes.func,
      ...togglePropTypes('expanded'),
    },
    defaultProps: {
      className: '',
      selectedValues: [],
      selectedOptionsDisplayFormatter: getLabel,
    }
  }
}

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.

5 participants