Skip to content

feat(Dropdown): Add support for dropleft and dropright (#785)#813

Merged
TheSharpieOne merged 3 commits into
reactstrap:masterfrom
supergibbs:dropdown-directions
Feb 7, 2018
Merged

feat(Dropdown): Add support for dropleft and dropright (#785)#813
TheSharpieOne merged 3 commits into
reactstrap:masterfrom
supergibbs:dropdown-directions

Conversation

@supergibbs

Copy link
Copy Markdown
Collaborator

Add support for dropleft and dropright (#785)

@TheSharpieOne TheSharpieOne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just some small nits to help reduce the amount of code.

Comment thread src/Dropdown.js Outdated
const propTypes = {
disabled: PropTypes.bool,
dropup: PropTypes.bool,
dropup: deprecated(PropTypes.bool, 'Please use the prop "direction"'),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would also indicate the using the value "up": 'Please use the prop "direction" with the value "up".'

Comment thread src/Dropdown.js Outdated
dropdown: !group && !addonType,
show: isOpen,
dropup: dropup,
dropup: direction === 'up',

@TheSharpieOne TheSharpieOne Feb 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These three lines can be

direction !== 'down' && `drop${direction}`

Comment thread src/DropdownMenu.js Outdated
const position1 = context.dropup ? 'top' : 'bottom';

let position1;
switch (context.direction) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use a hashmap instead and it will be more performant and less code.

// this defined outside of the component:
const directionPositionMap = {
  up: 'top',
  left: 'left',
  right: 'right',
  down: 'bottom',
};
// this right here:
const position1 = directionPositionMap[context.direction] || 'bottom';

You don't really need the || 'bottom' as the contextTypes should ensure it is in the hashmap.... but just in case?

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