Fix https://github.com/readmeio/api-explorer/issues/42#48
Fix https://github.com/readmeio/api-explorer/issues/42#48domharrington merged 15 commits intomasterfrom
Conversation
|
When auth warning is shown, then focus the auth input and scroll into view. Right now if the auth box is visible and the cursor is typed anywhere, the focus is being stoled into the auth box. |
| const { auth } = this.props; | ||
| return ( | ||
| <div className="row"> | ||
| <div className="row" ref={input => (this.input = input)}> |
| <div className="col-xs-6"> | ||
| <label htmlFor="user">username</label> | ||
| <input | ||
| ref={input => input && auth && input.focus()} |
There was a problem hiding this comment.
Is there ever a possibility that input isn't going to be defined here? Do we need to do this part: input &&?
There was a problem hiding this comment.
What's the best way to test this?
There was a problem hiding this comment.
if input && isn't there it cannot read focus of null. I got it from the second response since Oauth2 and ApiKey aren't classes https://stackoverflow.com/questions/28889826/react-set-focus-on-input-after-render
|
|
||
| Basic.propTypes = { | ||
| change: PropTypes.func.isRequired, | ||
| auth: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
In this context, naming this variable auth doesn't really mean too much? Would it make more sense to call it focus or something?
| switch (props.scheme.type) { | ||
| case 'oauth2': | ||
| return <Oauth2 {...props} change={change} />; | ||
| return <Oauth2 {...props} change={change} auth={props.auth} />; |
There was a problem hiding this comment.
This should already get passed down via the {...props}? or am i misunderstanding?
| const onChange = jest.fn(); | ||
| const securityInput = mount(<SecurityInput {...props} onChange={onChange} />); | ||
| const securityInput = mount( | ||
| <SecurityInput {...props} onChange={onChange} scrollIntoView={scrollIntoView} />, |
There was a problem hiding this comment.
Why do you need to pass scrollIntoView into the SecurityInput? It's not a prop in there is it?
|
If the element is already in the view, calling |
|
Although we dont have a direct test for this, the code path gets hit by this test: api-explorer/packages/api-explorer-ui/__tests__/Doc.test.jsx Lines 68 to 77 in 6579744 I dont particularly like all the wiring with the references everywhere, but i think it's okay for now until we add a better state management library possibly, although i dont even know if that would help with our component heirarchy. |
2 participants
No description provided.