-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ var React = require('react'); | |
var sdk = require('matrix-react-sdk') | ||
var dis = require('matrix-react-sdk/lib/dispatcher'); | ||
var rate_limited_func = require('matrix-react-sdk/lib/ratelimitedfunc'); | ||
var AccessibleButton = require('matrix-react-sdk/lib/components/views/elements/AccessibleButton'); | ||
|
||
module.exports = React.createClass({ | ||
displayName: 'SearchBox', | ||
|
@@ -35,6 +36,25 @@ module.exports = React.createClass({ | |
}; | ||
}, | ||
|
||
componentDidMount: function() { | ||
this.dispatcherRef = dis.register(this.onAction); | ||
}, | ||
|
||
componentWillUnmount: function() { | ||
dis.unregister(this.dispatcherRef); | ||
}, | ||
|
||
onAction: function(payload) { | ||
switch (payload.action) { | ||
// Clear up the text field when a room is selected. | ||
case 'view_room': | ||
if (this.refs.search) { | ||
this._clearSearch(); | ||
} | ||
break; | ||
} | ||
}, | ||
|
||
onChange: function() { | ||
if (!this.refs.search) return; | ||
this.setState({ searchTerm: this.refs.search.value }); | ||
|
@@ -61,35 +81,42 @@ module.exports = React.createClass({ | |
} | ||
}, | ||
|
||
_clearSearch: function() { | ||
this.refs.search.value = ""; | ||
this.onChange(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should happen automatically? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're kinda right, for reasons that are ... non-obvious. In case you're interested, and for my own reference: Essentially the React actually listens for the What we ought to do here is stick with the react side, and just make the change through react:
That will trigger a re-render, which will update the DOM. See also: https://facebook.github.io/react/docs/forms.html#controlled-components. |
||
}, | ||
|
||
render: function() { | ||
var TintableSvg = sdk.getComponent('elements.TintableSvg'); | ||
|
||
var collapseTabIndex = this.refs.search && this.refs.search.value !== "" ? "-1" : "0"; | ||
|
||
var toggleCollapse; | ||
if (this.props.collapsed) { | ||
toggleCollapse = | ||
<div className="mx_SearchBox_maximise" onClick={ this.onToggleCollapse.bind(this, true) }> | ||
<AccessibleButton className="mx_SearchBox_maximise" tabIndex={collapseTabIndex} onClick={ this.onToggleCollapse.bind(this, true) }> | ||
<TintableSvg src="img/maximise.svg" width="10" height="16" alt="<"/> | ||
</div> | ||
</AccessibleButton> | ||
} | ||
else { | ||
toggleCollapse = | ||
<div className="mx_SearchBox_minimise" onClick={ this.onToggleCollapse.bind(this, false) }> | ||
<AccessibleButton className="mx_SearchBox_minimise" tabIndex={collapseTabIndex} onClick={ this.onToggleCollapse.bind(this, false) }> | ||
<TintableSvg src="img/minimise.svg" width="10" height="16" alt="<"/> | ||
</div> | ||
</AccessibleButton> | ||
} | ||
|
||
var searchControls; | ||
if (!this.props.collapsed) { | ||
searchControls = [ | ||
this.state.searchTerm.length > 0 ? | ||
<div key="button" | ||
className="mx_SearchBox_closeButton" | ||
onClick={ ()=>{ this.refs.search.value = ""; this.onChange(); } }> | ||
<AccessibleButton key="button" | ||
className="mx_SearchBox_closeButton" | ||
onClick={ ()=>{ this._clearSearch(); } }> | ||
<TintableSvg | ||
className="mx_SearchBox_searchButton" | ||
src="img/icons-close.svg" width="24" height="24" | ||
/> | ||
</div> | ||
</AccessibleButton> | ||
: | ||
<TintableSvg | ||
key="button" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the divs still needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't want to touch it just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it'd be fine if the class were to be moved to the button element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of the divs.