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

Refactor Dropdowns #125

Merged
merged 14 commits into from
May 14, 2019
Merged

Refactor Dropdowns #125

merged 14 commits into from
May 14, 2019

Conversation

alexpaxton
Copy link
Contributor

@alexpaxton alexpaxton commented May 14, 2019

Closes #126

Screen Shot 2019-05-14 at 9 04 32 AM

Overview

Dropdowns have been broken down into a set of components that are indepenent of each other and only worry about individual aspects of the dropdown's behavior. Rather than a super customizable Dropdown component we have exposed all the inner workings to be customized directly. In pursuit of the Family / Composed pattern found in some other Clockface components the Dropdown family is now completely transparent and composable. This unfortunately is a breaking change to Dropdowns and will cause some tech debt.

Changes

  • Dropdown has 2 render props (Button and Menu) and only handles opening/closing the dropdown
  • Dropdown no longer cares about what its children are
  • Broke out DropdownMenu into own component
  • DropdownMenuColors --> DropdownMenuTheme
  • DropdownItem has a type prop which controls appearance when selected and can be None | Dot | Checkbox
  • DropdownItem has selected and onClick required
  • Dropdown no longer passes down any props or functions to DropdownItems so they must be manually wired up
  • Content of DropdownButton can be whatever you want
  • Created SelectDropdown that behaves like the HTML Select component as a composed variation of the Dropdown
  • Created MultiSelectDropdown as another composed variation of Dropdown
  • Created DropdownLinkItem for using <a> or <Link> as dropdown items

Future Work

#127
#61

return emptyText
}

private get menuOptions(): JSX.Element[] {
Copy link

@OfTheDelmer OfTheDelmer May 14, 2019

Choose a reason for hiding this comment

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

Should this be its own component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I'd say yes but in this case I like having the "composed" components as a single file

const {className} = this.props

return classnames('dropdown', {
className: className,

Choose a reason for hiding this comment

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

Suggested change
className: className,
className,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah looks like I have to do [${className}]: className because that coerces className to be of type string. classnames does not like that className is of type string | undefined. This might be a larger change since all clockface components handle className this way.

I created an issue for this: #128

}

export class DropdownDivider extends Component<Props> {
public static defaultProps = {
testID: 'dropdown--divider',
testID: 'dropdown-divider',
}

Choose a reason for hiding this comment

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

newline

className={classnames('dropdown--divider', {line: !text})}
className={classnames('dropdown-divider', {
line: !text,
[`${className}`]: className,

Choose a reason for hiding this comment

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

Suggested change
[`${className}`]: className,
[className]: className,


if (checkbox) {
if (type === DropdownItemType.Checkbox) {

Choose a reason for hiding this comment

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

could be a good switch case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup yup good call

const {theme} = this.props

switch (theme) {
case DropdownMenuTheme.Malachite:

Choose a reason for hiding this comment

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

I feel like this would be a good constant to extract where a theme enum key's a thumb colors object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These menu "themes" might not live that much longer as we eventually plan to use the Gradients enum instead

@alexpaxton alexpaxton merged commit 0e3c047 into master May 14, 2019
@alexpaxton alexpaxton deleted the refactor/dropdowns branch May 14, 2019 19:59
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.

Break down dropdowns
2 participants