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

Typescript and docs #794

Merged
merged 10 commits into from Apr 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .babelrc.js
@@ -1,4 +1,4 @@
module.exports = api => ({
module.exports = (api) => ({
presets: [
[
'@babel/env',
Expand All @@ -11,6 +11,7 @@ module.exports = api => ({
},
],
'@babel/react',
'@babel/preset-typescript',
],
plugins: [
['@babel/plugin-proposal-class-properties', { loose: true }],
Expand Down
12 changes: 10 additions & 2 deletions .eslintrc
@@ -1,8 +1,16 @@
{
"extends": ["@react-bootstrap"],
"extends": [
"@react-bootstrap",
"4catalyzer-typescript",
"prettier",
"prettier/react",
"prettier/@typescript-eslint"
],
"plugins": ["prettier"],
"rules": {
"jsx-a11y/no-autofocus": "off",
"prettier/prettier": "error"
"prettier/prettier": "error",
"import/extensions": "off",
"@typescript-eslint/no-empty-function": "off"
}
}
11 changes: 10 additions & 1 deletion karma.conf.js
Expand Up @@ -15,7 +15,16 @@ module.exports = (config) => {
webpack: {
mode: 'development',
module: {
rules: [rules.js()],
rules: [
{
...rules.js(),
test: /\.[jt]sx?$/,
},
],
},
resolve: {
symlinks: false,
extensions: ['.mjs', '.js', '.ts', '.tsx', '.json'],
},
plugins: [
plugins.define({
Expand Down
16 changes: 13 additions & 3 deletions package.json
Expand Up @@ -30,11 +30,11 @@
"scripts": {
"bootstrap": "yarn && yarn --cwd www",
"build": "rimraf lib && yarn build:cjs && yarn build:esm && yarn build:pick",
"build:cjs": "babel src -d lib/cjs && rollup src/popper.js --file lib/cjs/popper.js --format cjs --name popper --plugin @rollup/plugin-node-resolve",
"build:cjs": "babel src -d lib/cjs && rollup src/popper.ts --file lib/cjs/popper.js --format cjs --name popper --plugin @rollup/plugin-node-resolve",

Choose a reason for hiding this comment

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

mmaguebo

"build:esm": "babel src -d lib/esm --env-name esm",
"build:pick": "cherry-pick ../src --cjs-dir=cjs --esm-dir=esm --cwd=lib",
"deploy-docs": "yarn --cwd www build --prefix-paths && gh-pages -d www/public",
"lint": "eslint www/*.js www/src src test *.js",
"lint": "eslint www/*.js www/src src test *.js --ext .js,.ts,.tsx",
"prepublishOnly": "yarn build",
"release": "rollout",
"start": "yarn --cwd www start",
Expand All @@ -48,7 +48,7 @@
}
},
"lint-staged": {
"*.js": "eslint --fix"
"*.js,*.tsx": "eslint --fix --ext .js,.ts,.tsx"
},
"prettier": {
"singleQuote": true,
Expand All @@ -64,6 +64,7 @@
"@babel/runtime": "^7.4.5",
"@popperjs/core": "^2.0.0",
"@restart/hooks": "^0.3.12",
"@types/warning": "^3.0.0",
"dom-helpers": "^5.1.0",
"prop-types": "^15.7.2",
"uncontrollable": "^7.0.0",
Expand All @@ -75,15 +76,22 @@
},
"devDependencies": {
"@4c/rollout": "^2.1.6",
"@4c/tsconfig": "^0.3.1",
"@babel/cli": "^7.8.4",
"@babel/core": "^7.9.0",
"@babel/plugin-proposal-class-properties": "^7.8.3",
"@babel/plugin-transform-runtime": "^7.9.0",
"@babel/polyfill": "^7.8.7",
"@babel/preset-env": "^7.9.5",
"@babel/preset-react": "^7.9.4",
"@babel/preset-typescript": "^7.9.0",
"@react-bootstrap/eslint-config": "^1.3.2",
"@rollup/plugin-node-resolve": "^7.1.3",
"@types/classnames": "^2.2.10",
"@types/react": "^16.9.32",
"@types/react-dom": "^16.9.6",
"@typescript-eslint/eslint-plugin": "^2.27.0",
"@typescript-eslint/parser": "^2.27.0",
"babel-eslint": "^10.1.0",
"babel-plugin-add-module-exports": "^1.0.2",
"babel-plugin-istanbul": "^6.0.0",
Expand All @@ -93,6 +101,7 @@
"enzyme": "^3.11.0",
"enzyme-adapter-react-16": "^1.15.2",
"eslint": "^6.8.0",
"eslint-config-4catalyzer-typescript": "^1.1.8",
"eslint-config-prettier": "^6.10.1",
"eslint-plugin-import": "^2.20.2",
"eslint-plugin-jsx-a11y": "^6.2.3",
Expand Down Expand Up @@ -123,6 +132,7 @@
"simulant": "^0.2.2",
"sinon": "^9.0.2",
"sinon-chai": "^3.5.0",
"typescript": "^3.8.3",
"webpack": "^4.42.1",
"webpack-atoms": "^12.1.0",
"webpack-cli": "^3.3.11"
Expand Down
71 changes: 38 additions & 33 deletions src/Dropdown.js → src/Dropdown.tsx
Expand Up @@ -2,13 +2,13 @@ import matches from 'dom-helpers/matches';
import qsa from 'dom-helpers/querySelectorAll';
import React, { useCallback, useRef, useEffect, useMemo } from 'react';
import PropTypes from 'prop-types';
import { useUncontrolled } from 'uncontrollable';
import { useUncontrolledProp } from 'uncontrollable';
import usePrevious from '@restart/hooks/usePrevious';
import useCallbackRef from '@restart/hooks/useCallbackRef';
import useForceUpdate from '@restart/hooks/useForceUpdate';
import useEventCallback from '@restart/hooks/useEventCallback';

import DropdownContext from './DropdownContext';
import DropdownContext, { DropDirection } from './DropdownContext';
import DropdownMenu from './DropdownMenu';
import DropdownToggle from './DropdownToggle';

Expand Down Expand Up @@ -46,7 +46,7 @@ const propTypes = {
* Selectors should be relative to the menu component:
* e.g. ` > li:not('.disabled')`
*/
itemSelector: PropTypes.string.isRequired,
itemSelector: PropTypes.string,

/**
* Align the menu to the 'end' side of the placement side of the Dropdown toggle. The default placement is `top-start` or `bottom-start`.
Expand All @@ -69,7 +69,7 @@ const propTypes = {
* A callback fired when the Dropdown wishes to change visibility. Called with the requested
* `show` value, the DOM event, and the source that fired it: `'click'`,`'keydown'`,`'rootClose'`, or `'select'`.
*
* ```js
* ```ts static
Copy link
Member

Choose a reason for hiding this comment

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

what does static mean here?

Copy link
Member Author

Choose a reason for hiding this comment

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

not editable in place

* function(
* isOpen: boolean,
* event: SyntheticEvent,
Expand All @@ -81,46 +81,51 @@ const propTypes = {
onToggle: PropTypes.func,
};

const defaultProps = {
itemSelector: '* > *',
};
export interface DropdownInjectedProps {
onKeyDown: React.KeyboardEventHandler;
}

export interface DropdownProps {
drop?: DropDirection;
alignEnd?: boolean;
defaultShow?: boolean;
show?: boolean;
onToggle: (nextShow: boolean, event?: React.SyntheticEvent) => void;
itemSelector?: string;
focusFirstItemOnShow?: false | true | 'keyboard';
children: (arg: { props: DropdownInjectedProps }) => React.ReactNode;
}

/**
* `Dropdown` is set of structural components for building, accessible dropdown menus with close-on-click,
* keyboard navigation, and correct focus handling. As with all the react-overlay's
* components its BYOS (bring your own styles). Dropdown is primarily
* built from three base components, you should compose to build your Dropdowns.
*
* - `Dropdown`, which wraps the menu and toggle, and handles keyboard navigation
* - `Dropdown.Toggle` generally a button that triggers the menu opening
* - `Dropdown.Menu` The overlaid, menu, positioned to the toggle with PopperJs
* @displayName Dropdown
*/
function Dropdown({
drop,
alignEnd,
defaultShow,
show: rawShow,
onToggle: rawOnToggle,
itemSelector,
itemSelector = '* > *',
focusFirstItemOnShow,
children,
}) {
}: DropdownProps) {
const forceUpdate = useForceUpdate();
const { show, onToggle } = useUncontrolled(
{ defaultShow, show: rawShow, onToggle: rawOnToggle },
{ show: 'onToggle' },
const [show, onToggle] = useUncontrolledProp(
rawShow,
defaultShow!,
Copy link
Member

Choose a reason for hiding this comment

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

non-null assertion here seems somewhat wrong? this is potentially legitimately null-y, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can, the bang is just to make the type correct without specifying the generic

Copy link
Member

Choose a reason for hiding this comment

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

non-null assertion here seems somewhat wrong? this is potentially legitimately null-y, no?

rawOnToggle,
);

const [toggleElement, setToggle] = useCallbackRef();
const [toggleElement, setToggle] = useCallbackRef<HTMLElement>();

// We use normal refs instead of useCallbackRef in order to populate the
// the value as quickly as possible, otherwise the effect to focus the element
// may run before the state value is set
const menuRef = useRef();
const menuRef = useRef<HTMLElement | null>(null);
const menuElement = menuRef.current;

const setMenu = useCallback(
(ref) => {
(ref: null | HTMLElement) => {
menuRef.current = ref;
// ensure that a menu set triggers an update for consumers
forceUpdate();
Expand All @@ -129,7 +134,7 @@ function Dropdown({
);

const lastShow = usePrevious(show);
const lastSourceEvent = useRef(null);
const lastSourceEvent = useRef<string | null>(null);
const focusInDropdown = useRef(false);

const toggle = useCallback(
Expand Down Expand Up @@ -185,12 +190,12 @@ function Dropdown({

if (
focusType === false ||
(focusType === 'keyboard' && !/^key.+$/.test(type))
(focusType === 'keyboard' && !/^key.+$/.test(type!))
) {
return;
}

let first = qsa(menuRef.current, itemSelector)[0];
const first = qsa(menuRef.current!, itemSelector)[0];
if (first && first.focus) first.focus();
});

Expand All @@ -207,19 +212,20 @@ function Dropdown({
lastSourceEvent.current = null;
});

const getNextFocusedChild = (current, offset) => {
const getNextFocusedChild = (current: HTMLElement, offset: number) => {
if (!menuRef.current) return null;

let items = qsa(menuRef.current, itemSelector);
const items = qsa(menuRef.current, itemSelector);

let index = items.indexOf(current) + offset;
index = Math.max(0, Math.min(index, items.length));

return items[index];
};

const handleKeyDown = (event) => {
const { key, target } = event;
const handleKeyDown = (event: React.KeyboardEvent) => {
const { key } = event;
const target = event.target as HTMLElement;

// Second only to https://github.com/twbs/bootstrap/blob/8cfbf6933b8a0146ac3fbc369f19e520bd1ebdac/js/src/dropdown.js#L400
// in inscrutability
Expand All @@ -238,7 +244,7 @@ function Dropdown({

switch (key) {
case 'ArrowUp': {
let next = getNextFocusedChild(target, -1);
const next = getNextFocusedChild(target, -1);
if (next && next.focus) next.focus();
event.preventDefault();

Expand All @@ -249,7 +255,7 @@ function Dropdown({
if (!show) {
toggle(event);
} else {
let next = getNextFocusedChild(target, 1);
const next = getNextFocusedChild(target, 1);
if (next && next.focus) next.focus();
}
return;
Expand All @@ -271,7 +277,6 @@ function Dropdown({
Dropdown.displayName = 'ReactOverlaysDropdown';

Dropdown.propTypes = propTypes;
Dropdown.defaultProps = defaultProps;

Dropdown.Menu = DropdownMenu;
Dropdown.Toggle = DropdownToggle;
Expand Down
13 changes: 0 additions & 13 deletions src/DropdownContext.js

This file was deleted.

19 changes: 19 additions & 0 deletions src/DropdownContext.ts
@@ -0,0 +1,19 @@
import React from 'react';

export type DropDirection = 'up' | 'down' | 'left' | 'right';

export type DropdownContextValue = {
toggle: (nextShow: boolean, event?: React.SyntheticEvent | Event) => void;
menuElement: HTMLElement | null;
toggleElement: HTMLElement | null;
setMenu: (ref: HTMLElement | null) => void;
setToggle: (ref: HTMLElement | null) => void;

show: boolean;
alignEnd?: boolean;
drop?: DropDirection;
};

const DropdownContext = React.createContext<DropdownContextValue | null>(null);
Copy link
Member

Choose a reason for hiding this comment

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

might be safer to do null as any? i mean, we don't really deal with the case where the context isn't available, right? no point being robust to that

Copy link
Member Author

Choose a reason for hiding this comment

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

needed for examples and things where we just show e.g. the Dropdown Menu


export default DropdownContext;