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

Better user interface for screen readers and keyboard navigation #2946

Merged
merged 1 commit into from Feb 3, 2017

Conversation

@JaniM
Copy link
Contributor

JaniM commented Jan 13, 2017

Changed most button-like divs to actually behave as such (via the new AccessibleButton element). Also made filtering rooms more keyboard friendly.

Signed-off-by: Jani Mustonen janijohannes@kapsi.fi

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Jan 13, 2017

can you rebase this on latest develop so it doesn't include a load of unrelated changes?

@JaniM JaniM changed the base branch from master to develop Jan 13, 2017
@JaniM JaniM force-pushed the JaniM:hotkey-ux branch 2 times, most recently from b2d1505 to 410e685 Jan 13, 2017
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Jan 13, 2017

@JaniM JaniM force-pushed the JaniM:hotkey-ux branch from 410e685 to 5edb5f6 Jan 14, 2017
Copy link
Member

richvdh left a comment

a couple of nits it would be nice to fix

}

}

if (this.props.roomId) {
buttonGroup =
<div className="mx_RightPanel_headerButtonGroup">
<div className="mx_RightPanel_headerButton" title="Members" onClick={ this.onMemberListButtonClick }>
<div className="mx_RightPanel_headerButton">

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 24, 2017

Member

are the divs still needed here?

This comment has been minimized.

Copy link
@JaniM

JaniM Jan 25, 2017

Author Contributor

Didn't want to touch it just in case.

This comment has been minimized.

Copy link
@JaniM

JaniM Jan 25, 2017

Author Contributor

Seems like it'd be fine if the class were to be moved to the button element.

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 3, 2017

Member

I got rid of the divs.

@@ -61,35 +81,42 @@ module.exports = React.createClass({
}
},

_clearSearch: function() {
this.refs.search.value = "";
this.onChange();

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 24, 2017

Member

this should happen automatically?

This comment has been minimized.

Copy link
@JaniM

JaniM Jan 25, 2017

Author Contributor

Doesn't.

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 3, 2017

Member

You're kinda right, for reasons that are ... non-obvious. In case you're interested, and for my own reference:

Essentially the onChange callback is a React fabrication. Native-DOM <input> elements have an onchange attribute, but it is only called when the element loses focus.

React actually listens for the input event, and calls the onChange callback when it sees one. Apparently the DOM does not fire an input event when the value is changed from js - which is probably lucky otherwise React would have to jump through hoops to avoid getting stuck in a loop.

What we ought to do here is stick with the react side, and just make the change through react:

this.setState({ searchTerm: "" });

That will trigger a re-render, which will update the DOM.

See also: https://facebook.github.io/react/docs/forms.html#controlled-components.

@tessgadwa tessgadwa referenced this pull request Jan 24, 2017
16 of 71 tasks complete
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Feb 2, 2017

@JaniM hang on - i'm confused. i thought this had already landed? or did we only merge the react-sdk half of it?

@richvdh richvdh merged commit 5edb5f6 into vector-im:develop Feb 3, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.