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.

Sign up for GitHub

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

Jump to bottom

Prepare gestalt for eslint 5 via bump to plugins #273

Merged
merged 9 commits into from
Jul 11, 2018
Merged

Prepare gestalt for eslint 5 via bump to plugins #273

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da64e18
Bumping eslint and fixing errors
Jul 11, 2018
97ebed4
Disable rule
Jul 11, 2018
64ba012
Changelog
Jul 11, 2018
6336e59
Snapshot
Jul 11, 2018
b39a9e3
Fix one flow error
Jul 11, 2018
5932f01
Try * type
Jul 11, 2018
47483f5
Add flow issue instead
Jul 11, 2018
0f0bf30
Revert Button changes
Jul 11, 2018
0bdccd7
Change one more button type
Jul 11, 2018
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
1 change: 1 addition & 0 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
],
"no-await-in-loop": "off",
"no-return-await": "off",
"react/destructuring-assignment": "off",
"react/jsx-filename-extension": "off",
"react/no-array-index-key": "off",
"react/require-default-props": "off",
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* Icon: Fix `envelope` icon + add `gmail` (#270)
* Internal: Fix release script for gh-pages (#266)
* Flow: Bump Flow to version 0.75.0 in gestalt (#268)
* Internal: Bump all eslint plugin packages in prep for eslint5 (#273)

</details>

Expand Down
11 changes: 6 additions & 5 deletions docs/src/Masonry.doc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,6 @@ class ExampleMasonry extends React.Component<
});
}

// ref on a component gets the mounted instance of the component
// https://reactjs.org/docs/refs-and-the-dom.html#adding-a-ref-to-a-class-component
grid: ?Masonry<*>;
scrollContainer: ?HTMLElement;

// eslint-disable-next-line react/no-unused-prop-types
updateWidth = ({ target }: { target: HTMLInputElement }) => {
this.setState({ width: Number(target.value) }, () => {
Expand All @@ -198,6 +193,12 @@ class ExampleMasonry extends React.Component<
});
};

// ref on a component gets the mounted instance of the component
// https://reactjs.org/docs/refs-and-the-dom.html#adding-a-ref-to-a-class-component
grid: ?Masonry<*>;

scrollContainer: ?HTMLElement;

render() {
const containerStyle = {
height: '300px',
Expand Down
1 change: 1 addition & 0 deletions docs/src/components/PropTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export default function PropTable({ props: properties, Component }: Props) {
const hasRequired = properties.some(prop => prop.required);

if (process.env.NODE_ENV === 'development' && Component) {
// eslint-disable-next-line react/forbid-foreign-prop-types
const { displayName, propTypes } = Component;
const missingProps = Object.keys(propTypes || {}).reduce((acc, prop) => {
if (!properties.find(p => p.name === prop)) {
Expand Down
18 changes: 9 additions & 9 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,17 @@
"danger": "^3.1.7",
"enzyme": "^3.3.0",
"enzyme-adapter-react-16": "^1.1.1",
"eslint": "^4.17.0",
"eslint-config-airbnb": "^16.1.0",
"eslint": "^4.19.1",
"eslint-config-airbnb": "^17.0.0",
"eslint-config-postcss": "^2.0.2",
"eslint-config-prettier": "^2.1.1",
"eslint-plugin-eslint-comments": "^2.0.2",
"eslint-plugin-flowtype": "^2.34.0",
"eslint-config-prettier": "^2.9.0",
"eslint-plugin-eslint-comments": "^3.0.1",
"eslint-plugin-flowtype": "^2.50.0",
"eslint-plugin-import": "^2.13.0",
"eslint-plugin-jest": "^21.15.1",
"eslint-plugin-jsx-a11y": "^6.0.2",
"eslint-plugin-prettier": "^2.1.1",
"eslint-plugin-react": "^7.4.0",
"eslint-plugin-jest": "^21.17.0",
"eslint-plugin-jsx-a11y": "^6.1.0",
"eslint-plugin-prettier": "^2.6.2",
"eslint-plugin-react": "^7.10.0",
"filesize": "^3.6.1",
"flow-bin": "0.75.0",
"husky": "^0.14.3",
Expand Down
3 changes: 2 additions & 1 deletion packages/gestalt-codemods/0.33.0-0.34.0/avatar-non-responsive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ export default function transformer(file, api) {
if (convertSizes[value]) {
attribute.value.value = convertSizes[value];
return attribute;
} else if (invalidSizes.includes(value)) {
}
if (invalidSizes.includes(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wow huh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrislloyd same reaction haha, its this rule https://eslint.org/docs/rules/no-else-return which does not like an else if clause after a return

throw new Error(
`
${file.path}
Expand Down
18 changes: 14 additions & 4 deletions packages/gestalt/src/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ export default function Button(props: Props) {
accessibilityHaspopup,
accessibilityLabel,
color = 'gray',
disabled = false,
inline = false,
disabled,
inline,
name,
onClick,
size = 'md',
size,
text,
type = 'button',
type,
} = props;

const textColor = {
Expand All @@ -54,6 +54,7 @@ export default function Button(props: Props) {
[styles.block]: !inline,
});

/* eslint-disable react/button-has-type */
Copy link
Contributor

Choose a reason for hiding this comment

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

@pfarejowicz Any idea why we need to disable this even though we set it below? Is it because type is dynamic?

type={type}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christianvuerings exactly! the rule does not like us using the dynamic value type={type} even if it is Flow typed and prop typed to match the possible values. There's more discussion in jsx-eslint/eslint-plugin-react#1555 and the recommendation is to fork the component and render two different JSX components but to me that feels like overkill, what are your thoughts? For reference:

I'd say that the rule should be forcing a single, hardcoded type value - and that you should conditionally render two different elements if you want two types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Disabling it here seems fine - thanks for clarifying

return (
<button
aria-expanded={accessibilityExpanded}
Expand All @@ -76,8 +77,17 @@ export default function Button(props: Props) {
</Text>
</button>
);
/* eslint-enable react/button-has-type */
}

Button.defaultProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm so torn on this one. None of our tools really introspect defaultProps and it's not a required part of the React API so this definitely feels like extra typing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed - I started off by playing with the typing and defaulting, hoping to fix the type={type} error but to no avail since I found the github thread linked above saying it needs to be hardcoded to "button" or "submit". Figured I would keep the defaultProps that I created anyways since it is a common pattern. Thoughts? I can revert the defaulting since the rule needs to be disabled anyways

color: 'gray',
disabled: false,
inline: false,
size: 'md',
type: 'button',
};

Button.propTypes = {
accessibilityExpanded: PropTypes.bool,
accessibilityHaspopup: PropTypes.bool,
Expand Down
7 changes: 4 additions & 3 deletions packages/gestalt/src/Checkbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export default class Checkbox extends React.Component<Props, State> {
size: 'md',
};

state: State = {
state = {
focused: false,
};

Expand All @@ -60,16 +60,17 @@ export default class Checkbox extends React.Component<Props, State> {
}
}

input: ?HTMLInputElement;

handleChange = (event: SyntheticInputEvent<>) => {
const { checked } = event.target;
this.props.onChange({ event, checked });
};

handleBlur = () => this.setState({ focused: false });

handleFocus = () => this.setState({ focused: true });

input: ?HTMLInputElement;

render() {
const { checked, disabled, id, indeterminate, name, size } = this.props;
return (
Expand Down
6 changes: 5 additions & 1 deletion packages/gestalt/src/Flyout.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import Flyout from './Flyout.js';
it('Flyout renders', () => {
const wrapper = shallow(
<Flyout
anchor={<button onClick={() => null}> test </button>}
anchor={
<button onClick={() => null} type="submit">
Copy link
Contributor

Choose a reason for hiding this comment

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

@pfarejowicz we should probably change this to type="button". A flyout inside of a form should not be submitting the form. Also button is the default for <Button>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change

test
</button>
}
idealDirection="down"
onDismiss={jest.fn()}
size="sm"
Expand Down
5 changes: 5 additions & 0 deletions packages/gestalt/src/IconButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,21 +41,26 @@ export default class IconButton extends React.Component<Props, State> {
};

handleBlur = () => this.setState({ focused: false });

handleFocus = () => {
this.setState({ focused: true });
};

handleMouseDown = () => {
this.setState({ active: true });
};

handleMouseEnter = () => {
this.setState({ hovered: true });
};

handleMouseLeave = () => {
this.setState({
active: false,
hovered: false,
});
};

handleMouseUp = () => {
this.setState({ active: false });
};
Expand Down
2 changes: 2 additions & 0 deletions packages/gestalt/src/Layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ export default class Layer extends React.Component<Props> {
super(props);
this.el = document.createElement('div');
}

componentDidMount() {
if (document.body) {
document.body.appendChild(this.el);
}
}

componentWillUnmount() {
if (document.body) {
document.body.removeChild(this.el);
Expand Down