Menu doesnt stay open on click #330

Merged
merged 11 commits into from Feb 16, 2017

Projects

None yet

3 participants

@AGS-
Member
AGS- commented Feb 15, 2017

Made select and open have more typical behavior.

@AGS-
Member
AGS- commented Feb 15, 2017

Fixing errors brought up by travis

@AGS-
Member
AGS- commented Feb 15, 2017

@wincent I was trying to pass in the DOM element by using ref however I couldn't figure out a good way to do it since I also need to pass in the event via this.handleClick.bind(this) to do the comparison. Any way I could pass both?

src/components/ToolbarMenu.js
+ this.handleClick.bind(this),
+ false
+ );
+ }}
@wincent
wincent Feb 15, 2017 Contributor

This isn't exactly how ref props are supposed to be used. The pattern is:

// In `render()`:
ref={node => this._node = node}

And then elsewhere (like in componentDidMount() or an event handler), access this._node instead of using ReactDOM.findDOMNode.

So your document.addEventListener should go in handleOpen, and your handleClick should be able to do an if (this._node !== e.target) or similar.

@AGS-
AGS- Feb 15, 2017 Member

I agree, that's a better pattern.

src/components/ToolbarMenu.js
- }
- }
+ handleClick(e) {
+ // eslint-disable-next-line no-undef
@wincent
wincent Feb 15, 2017 Contributor

You won't need this suppression if you don't use ReactDOM (and if you did have to use it, I think the right way to deal with the lint would be to import it).

@AGS-
AGS- Feb 15, 2017 Member

Haha yeah I felt bad about throwing the suppression in there

src/components/ToolbarSelect.js
- document.addEventListener('click', this.handleClick.bind(this), false);
+ document.addEventListener('click',
+ this.handleClick.bind(this),
+ false);
@wincent
wincent Feb 15, 2017 Contributor

Similar issue here; I don't think this is the pattern we want to follow.

src/components/ToolbarSelect.js
@@ -63,6 +70,7 @@ export class ToolbarSelect extends React.Component {
}
handleClick(e) {
+ // eslint-disable-next-line no-undef
@wincent
wincent Feb 15, 2017 Contributor

As above.

@wincent
Contributor
wincent commented Feb 15, 2017

Awesome. Thanks for fixing this. I left a comment or four inline but I think this is looking close. Thanks!

@AGS-
Member
AGS- commented Feb 15, 2017

@wincent Fixed!

+ <a
+ className="toolbar-menu toolbar-button"
+ onClick={this.handleOpen.bind(this)}
+ onMouseDown={preventDefault}
@wincent
wincent Feb 15, 2017 Contributor

(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.

@AGS-
AGS- Feb 15, 2017 Member

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

src/components/ToolbarMenu.js
+ handleOpen = e => {
+ e.preventDefault();
+ this.setState({ visibility: 'visible' });
+ document.addEventListener('click', this.handleClick.bind(this));
@wincent
wincent Feb 15, 2017 Contributor

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

src/components/ToolbarSelect.js
- onSelect && onSelect.bind(null, child.props.value, i);
- return <ToolbarSelectOption {...child.props} onSelect={onChildSelect} />;
- });
+ this.props.onSelect && this.props.onSelect.bind(null,
@wincent
wincent Feb 15, 2017 Contributor

Indentation here looks funky; line continuations should be indented 2 spaces.

src/components/ToolbarSelect.js
+ handleOpen = e => {
+ e.preventDefault();
+ this.setState({ visibility: 'visible' });
+ document.addEventListener('click', this.handleClick.bind(this));
@wincent
wincent Feb 15, 2017 Contributor

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

@AGS-
AGS- Feb 15, 2017 Member

Whoops!

src/components/ToolbarMenu.js
+
+ render() {
+ const ulStyle = {
+ visibility: this.state.visibility
@wincent
wincent Feb 15, 2017 Contributor

(Non-blocking feedback.)

I probably would have gone with a class here. It would result in a smaller change to the CSS file, and would mean we'd be more consistently using one method (predominantly class selectors) of applying presentation throughout the app.

@AGS-
AGS- Feb 15, 2017 Member

Alright, yeah I'll change this to use a css class instead.

@wincent
Contributor
wincent commented Feb 15, 2017

Left a few small comments but this is very close now. Would be good to fix those, then get this in; we can always iterate more on it later if we need.

@AGS-
Member
AGS- commented Feb 15, 2017

Alright, I'll update this PR in a bit.

@AGS-
Member
AGS- commented Feb 15, 2017

Alright, I made it use classname instead of style like we do in ToolbarButton. I also have it remove eventListeners now.

@AGS-
Member
AGS- commented Feb 15, 2017

graphiqlselectmenu

src/components/ToolbarMenu.js
@@ -40,7 +38,7 @@ export class ToolbarMenu extends React.Component {
<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" style={ulStyle}>
+ <ul className={'toolbar-menu-items' + (visible ? ' open' : '')}>
@wincent
wincent Feb 16, 2017 Contributor

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

src/components/ToolbarMenu.js
@@ -50,13 +48,14 @@ export class ToolbarMenu extends React.Component {
handleClick(e) {
if (this._node !== e.target) {
e.preventDefault();
- this.setState({ visibility: 'hidden' });
+ this.setState({ visible: false });
+ document.removeEventListener('click', this.handleClick.bind(this));
@wincent
wincent Feb 16, 2017 Contributor

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

@wincent
Contributor
wincent commented Feb 16, 2017

I think we're good to land this after the componentWillUnmount change that I commented on inline.

src/components/ToolbarMenu.js
@@ -25,6 +25,10 @@ export class ToolbarMenu extends React.Component {
this.state = { visible: false };
}
+ componentWillUnmount() {
+ document.removeEventListener('click', this.handleClick.bind(this));
@wincent
wincent Feb 16, 2017 Contributor

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.

@AGS-
AGS- Feb 16, 2017 Member

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

@wincent
Contributor
wincent commented Feb 16, 2017

Sweet!

@wincent wincent merged commit 80ee2ae into graphql:master Feb 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@AGS- AGS- deleted the AGS-:MenuDoesntStayOpenOnClick branch Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment