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

Jump to bottom

Menu doesnt stay open on click #330

Merged
merged 11 commits into from Feb 16, 2017
Merged

Menu doesnt stay open on click #330

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0dddb87
Change menu and select behavior to what is expected
AGS- Feb 15, 2017
ee5955e
Remove test elements from graphiql toolbar
AGS- Feb 15, 2017
06eb6e1
Get rid of leftover unused variables in menu
AGS- Feb 15, 2017
9957e8a
Make eslint happy
AGS- Feb 15, 2017
2d559c0
Use exposed node via ref instead of ReactDOM
AGS- Feb 15, 2017
4e05ef0
Remove test elements again
AGS- Feb 15, 2017
e572591
Using classname instead of style, removing orphan evenListeners
AGS- Feb 15, 2017
554378d
Make Usage of preventDefault uniform
AGS- Feb 15, 2017
ab03349
Fix lint error
AGS- Feb 15, 2017
88cdeea
Add eventListener removal in componentWillUnmount jic
AGS- Feb 16, 2017
bf71f4c
Change to make sure that the listener is removed
AGS- Feb 16, 2017
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
7 changes: 5 additions & 2 deletions css/app.css
View file
Open in desktop
Expand Up @@ -327,15 +327,18 @@
visibility: hidden;
}

.graphiql-container .toolbar-menu-items.open {
visibility: visible;
}

.graphiql-container .toolbar-select-options {
left: 0;
min-width: 100%;
top: -5px;
visibility: hidden;
}

.graphiql-container .toolbar-menu:active .toolbar-menu-items,
.graphiql-container .toolbar-select:active .toolbar-select-options {
.graphiql-container .toolbar-select-options.open {
visibility: visible;
}

Expand Down
68 changes: 48 additions & 20 deletions src/components/ToolbarMenu.js
View file
Open in desktop
Expand Up @@ -14,27 +14,55 @@ import React, { PropTypes } from 'react';
*
* A menu style button to use within the Toolbar.
*/
export function ToolbarMenu({ title, label, children }) {
return (
<a
className="toolbar-menu toolbar-button"
onMouseDown={preventDefault}
title={title}>
{label}
<svg width="14" height="8">
<path fill="#666" d="M 5 1.5 L 14 1.5 L 9.5 7 z" />
</svg>
<ul className="toolbar-menu-items">
{children}
</ul>
</a>
);
}
export class ToolbarMenu extends React.Component {
static propTypes = {
title: PropTypes.string,
label: PropTypes.string,
}

ToolbarMenu.propTypes = {
title: PropTypes.string,
label: PropTypes.string,
};
constructor(props) {
super(props);
this.state = { visible: false };
}

componentWillUnmount() {
document.removeEventListener('click', this.handleClick.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that any of these removeEventListener calls will do what you want, because when you call bind you get a new function that is not ===, which means that you won't remove anything here. (I could be wrong about that, but that's what I think will happen.)

I think the way to do this is as follows:

_subscribe() {
  if (!this._listener) {
    this._listener = this.handleClick.bind(this);
    document.addEventListener('click', this._listener);
  }
}

_release() {
  if (this._listener) {
    document.removeEventListener('click', this._listener);
    this._listener = null;
  }
}

And then call those methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, makes sense. Yeah I was afraid of that but I wasn't really sure. Implemented _subscribe and _release and tested that everything works.

}

render() {
const visible = this.state.visible;
return (
<a
className="toolbar-menu toolbar-button"
onClick={this.handleOpen.bind(this)}
onMouseDown={preventDefault}
Copy link
Contributor

Choose a reason for hiding this comment

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

(Non-blocking feedback.)

I think we should either just kill the preventDefault function, or use it everywhere. It's a bit of a smell to have two ways of preventing a default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah forgot to put this in, let me do another commit really quick

ref={node => {this._node = node;}}
title={this.props.title}>
{this.props.label}
<svg width="14" height="8">
<path fill="#666" d="M 5 1.5 L 14 1.5 L 9.5 7 z" />
</svg>
<ul className={'toolbar-menu-items' + (visible ? ' open' : '')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use the classnames package for this, but I think it's good to keep the dependency footprint small, so this is good.

{this.props.children}
</ul>
</a>
);
}

handleClick(e) {
if (this._node !== e.target) {
preventDefault(e);
this.setState({ visible: false });
document.removeEventListener('click', this.handleClick.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to also remove it, if present, in componentWillUnmount.

}
}

handleOpen = e => {
preventDefault(e);
this.setState({ visible: true });
document.addEventListener('click', this.handleClick.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

For hygiene, we should make sure we remove this event listener on unmount or close. The currently implementation will re-bind repeatedly.

};
}

export function ToolbarMenuItem({ onSelect, title, label }) {
return (
Expand Down
96 changes: 64 additions & 32 deletions src/components/ToolbarSelect.js
View file
Open in desktop
Expand Up @@ -14,42 +14,74 @@ import React, { PropTypes } from 'react';
*
* A select-option style button to use within the Toolbar.
*
* Note that, like React's <select>, this component is stateless and expects you
* to re-render with a new selected={} condition on the child options.
*/
export function ToolbarSelect({ onSelect, title, children }) {
let selectedChild;
const optionChildren = React.Children.map(children, (child, i) => {
if (!selectedChild || child.props.selected) {
selectedChild = child;
}
const onChildSelect =

export class ToolbarSelect extends React.Component {
static propTypes = {
title: PropTypes.string,
label: PropTypes.string,
onSelect: PropTypes.func
}

constructor(props) {
super(props);
this.state = { visible: false };
}

componentWillUnmount() {
document.removeEventListener('click', this.handleClick.bind(this));
}

render() {
let selectedChild;
const visible = this.state.visible;
const optionChildren = React.Children.map(this.props.children,
(child, i) => {
if (!selectedChild || child.props.selected) {
selectedChild = child;
}
const onChildSelect =
child.props.onSelect ||
onSelect && onSelect.bind(null, child.props.value, i);
return <ToolbarSelectOption {...child.props} onSelect={onChildSelect} />;
});
this.props.onSelect && this.props.onSelect.bind(null,
child.props.value,
i
);
return <ToolbarSelectOption {...child.props} onSelect={onChildSelect} />;
});
return (
<a
className="toolbar-select toolbar-button"
onClick={this.handleOpen.bind(this)}
onMouseDown={preventDefault}
ref={node => {this._node = node;}}
title={this.props.title}>
{selectedChild.props.label}
<svg width="13" height="10">
<path fill="#666" d="M 5 5 L 13 5 L 9 1 z" />
<path fill="#666" d="M 5 6 L 13 6 L 9 10 z" />
</svg>
<ul className={'toolbar-select-options' + (visible ? ' open' : '')}>
{optionChildren}
</ul>
</a>
);
}

return (
<a
className="toolbar-select toolbar-button"
onMouseDown={preventDefault}
title={title}>
{selectedChild.props.label}
<svg width="13" height="10">
<path fill="#666" d="M 5 5 L 13 5 L 9 1 z" />
<path fill="#666" d="M 5 6 L 13 6 L 9 10 z" />
</svg>
<ul className="toolbar-select-options">
{optionChildren}
</ul>
</a>
);
}
handleClick(e) {
if (this._node !== e.target) {
preventDefault(e);
this.setState({ visible: false });
document.removeEventListener('click', this.handleClick.bind(this));
}
}

ToolbarSelect.propTypes = {
onSelect: PropTypes.func,
title: PropTypes.string,
};
handleOpen = e => {
preventDefault(e);
this.setState({ visible: true });
document.addEventListener('click', this.handleClick.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue with unmount here. Should always have a removeEventListener to balance every addEventListener call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops!

};

}

export function ToolbarSelectOption({ onSelect, label, selected }) {
return (
Expand Down