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

Offcanvas has incorrect prop types for placement/direction #2511

Closed
BenJenkinson opened this issue Apr 29, 2022 · 0 comments · Fixed by #2512
Closed

Offcanvas has incorrect prop types for placement/direction #2511

BenJenkinson opened this issue Apr 29, 2022 · 0 comments · Fixed by #2512

Comments

@BenJenkinson
Copy link
Contributor

BenJenkinson commented Apr 29, 2022

  • components: Offcanvas
  • reactstrap version #9.0.2

What is happening?

👀 Offcanvas.js defines & uses the direction prop, and does not mention a placement prop:

direction: PropTypes.oneOf(['start', 'end', 'bottom', 'top', 'left', 'right']),

reactstrap/src/Offcanvas.js

Lines 412 to 414 in 1179b87

className={mapToCssModules(classNames('offcanvas', className,
`offcanvas-${direction}`
), cssModule)}

👀 OffcanvasProps.d.ts defines the placement prop, and does not define a direction prop:

export type Placement = 'start' | 'end' | 'left' | 'right' | 'bottom' | 'top';
export interface OffcanvasProps extends React.HTMLAttributes<HTMLElement> {
[key: string]: any;
autoFocus?: boolean;
backdrop?: boolean | 'static';
backdropClassName?: string;
backdropTransition?: FadeProps;
container?: string | HTMLElement | React.RefObject<HTMLElement>;
contentClassName?: string;
cssModule?: CSSModule;
fade?: boolean;
innerRef?: React.Ref<HTMLElement>;
isOpen?: boolean;
keyboard?: boolean;
labelledBy?: string;
offcanvasClassName?: string;
offcanvasTransition?: FadeProps;
onClosed?: () => void;
onEnter?: () => void;
onExit?: () => void;
onOpened?: () => void;
placement?: Placement;
returnFocusAfterClose?: boolean;
scrollable?: boolean;
toggle?: React.KeyboardEventHandler<any> | React.MouseEventHandler<any>;
trapFocus?: boolean;
unmountOnClose?: boolean;
wrapClassName?: string;
zIndex?: number | string;
}

👀 OffcanvasProps.d.ts defines the placement prop as including the values "left" | "right", but these are not valid options for the prop.

https://github.com/twbs/bootstrap/blob/7745730e4132eff204bf2afe8351511e595acee6/scss/_offcanvas.scss#L57-L90

User impact

"placement" prop appears in intellisense, but does not have any effect (prop should be "direction")

image

"direction" prop does not appear in intellisense (prop not defined in OffcanvasProps)

image

What should be happening?

The type definition for OffcanvasProps should match the implementation in Offcanvas.js.

I would suggest that since we use "placement" for tooltips, and the Bootstrap docs describe "placement", that we should change the implementation of Offcanvas.js to use the placement prop instead of the direction prop.

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 a pull request may close this issue.

1 participant