-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOCSP-4188: URI Writer #14
Conversation
725ccc3
to
245ba42
Compare
@@ -0,0 +1,267 @@ | |||
import React from 'react'; | |||
import { mount } from 'enzyme'; |
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.
We were thinking about using React testing library for the react component tests but I have to admit I like the way these tests look...the mocking is elegant.
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.
Ah sorry! I should have checked in first 😣 I am pretty familiar with Enzyme so I went with that—but am happy to update these if we want to go with the other library.
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.
No this is great, we can keep it.
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.
A couple of minor comments!
@@ -62,9 +73,12 @@ export default class GuideSection extends Component { | |||
return <ComponentFactory { ...this.props } | |||
nodeData={ child } | |||
key={ index } | |||
handleUpdateURIWriter={this.handleUpdateURIWriter.bind(this)} |
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.
Nit: Spaces around interior of curly braces
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 like spaces around interior of curly braces as well
@@ -62,9 +73,12 @@ export default class GuideSection extends Component { | |||
return <ComponentFactory { ...this.props } | |||
nodeData={ child } | |||
key={ index } | |||
handleUpdateURIWriter={this.handleUpdateURIWriter.bind(this)} |
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.
Optionally, use public class fields to avoid the need for .bind()
front-end/src/components/URIText.js
Outdated
const TEMPLATE_TYPE_ATLAS_34 = 'Atlas (Cloud) v. 3.4'; | ||
const TEMPLATE_TYPE_ATLAS = 'Atlas (Cloud)'; | ||
|
||
const URI_PLACEHOLDER = '<URISTRING>'; |
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.
Hm. We should probably single-source these constants between the two components
front-end/src/components/URIText.js
Outdated
return replacePlaceholderWithURI(value, templateType, uri); | ||
} | ||
|
||
const replacePlaceholderWithURI = (value, templateType, uri) => { |
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'm inclined to think that these should be regular function declarations
front-end/tests/URIText.test.js
Outdated
import URIText from '../src/components/URIText'; | ||
|
||
describe('local MongoDB', () => { | ||
const templateType = 'local MongoDB' |
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.
Missing semicolon
front-end/tests/URIText.test.js
Outdated
|
||
describe('when called without a placeholder', () => { | ||
it('returns the original string', () => { | ||
const value = `This is the body text`; |
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.
Use single-quotes unless you're actually formatting text
@i80and thanks! I've addressed your comments, ready for another look. |
* DOCSP-4188: URI Writer * Bugfix: implement fully uncontrolled component with key * Address andrew's feedback
[JIRA] This PR adds two components associated with the URI Writer:
URIWriter.js
: a form that allows user to input their own data, from which we generate connection stringsURIText.js
: replaces placeholders with the URI string. For now, we are only examining text that appears as aCode
component, but it would be easy to replace text in any component with a URI string.