OBPIH-851: Add base form fields components #278

pmuchowski · 2018-05-17T12:49:09Z

No description provided.

kkaczmarczyk · 2018-05-17T13:48:13Z

src/js/components/form-elements/SelectField.jsx

+ const delimiter = attributes.delimiter || ';';
+
+ return (
+ <div className="col-md-4">


there might be exceptions out of this, but let's see along the way. If so, we'll parametrize this later on.

can you elaborate what you mean by "exceptions"?

Oh wait are you talking about the CSS there? Yeah I assume we'll need to make that parameterizable at some point. Or just get rid of the div altogether and let the page wrap the inputs with divs using whatever column width they want.

And if you do need the div in the component we might just want to go with a col-md-12 so that we at least we have the ability to nest it in another div to change the width. I.e. we could nest this input component in a col-md-4 ourselves and the component would fill that div.

<div class='col-md-4'> <howeverYouReferenceAComponent/> </div>

Yup this was about the css part. I think for now it looks alright with forms. We can extend capabilities and customizability of these components in future tickets.

… tests

jmiranda

Looks good. Just asked a few questions to gain a better understanding of what's going on.

jmiranda · 2018-05-18T13:39:06Z

src/js/components/form-elements/SelectField.jsx

+ const delimiter = attributes.delimiter || ';';
+
+ return (
+ <div className="col-md-4">


can you elaborate what you mean by "exceptions"?

jmiranda · 2018-05-18T13:51:55Z

package.json

- ]
+ ],
+ "moduleNameMapper": {
+ "\\.(css|scss)$": "<rootDir>/src/js/tests/__mocks__/styleMock.js"


Ok, I'm probably going to start asking more questions now so I can absorb some of this. Can you explain what is going on here? From the docs it seems like we're stubbing out any css/scss files. But why is that needed? To prevent styles from contaminating the DOM (i.e. hiding a DOM element that is being tested)?

if you want to test component that imports some css with jest it will fail, because jest it's handling only javascript and you need to mock any other file types

jmiranda · 2018-05-18T13:58:08Z

src/js/components/form-elements/CheckboxField.jsx

+ return (
+ <BaseField
+ {...props}
+ renderInput={renderInput}


So let me try to get a better understanding of react components. Every react component has some type of render method. Is renderInput a convention or do input components need to use renderInput? This return is associated with the CheckboxField so I assume this is defining the component while renderInput is added to perform rendering operations on any type of event (mouseclick, etc). Where do we define which events get passed to this component? I assume that's where redux comes in?

renderInput it's just a parameter, there is no convention here I named it this way because it's actually rendering an "input". BaseField is adding some shared code to all of the fields (label, validation messages, etc.) and using this function to renter the input. It's done this way to have small and reusable components.

Sorry, that's exactly what I meant by "convention". So not necessarily a strict React standard but rather a convention that you've adopted.

jmiranda · 2018-05-18T14:00:44Z

src/js/components/form-elements/SelectField.jsx

+ const delimiter = attributes.delimiter || ';';
+
+ return (
+ <div className="col-md-4">


Oh wait are you talking about the CSS there? Yeah I assume we'll need to make that parameterizable at some point. Or just get rid of the div altogether and let the page wrap the inputs with divs using whatever column width they want.

jmiranda · 2018-05-18T14:01:19Z

src/js/components/form-elements/DateField.jsx

+ };
+
+ return (
+ <BaseReactField


What's the different between BaseReactField and BaseField?

BaseReactField is rendering BaseField it's some kind of child parent relation

jmiranda · 2018-05-18T14:08:10Z

src/js/components/form-elements/SelectField.jsx

+ const delimiter = attributes.delimiter || ';';
+
+ return (
+ <div className="col-md-4">


And if you do need the div in the component we might just want to go with a col-md-12 so that we at least we have the ability to nest it in another div to change the width. I.e. we could nest this input component in a col-md-4 ourselves and the component would fill that div.

<div class='col-md-4'> <howeverYouReferenceAComponent/> </div>

jmiranda · 2018-05-18T14:11:00Z

src/js/tests/.eslintrc

+{
+ "rules": {
+ "react/prop-types": 0,
+ "no-undef": 0


This appears like you are disabling noisy linting rules that you don't want to see while running tests? Is that correct? And is this just for test code? Is there a default set of linting rules defined somewhere?

yes, this is just for tests, the main eslint file in in the root directory

jmiranda · 2018-05-18T14:12:47Z

src/js/tests/form-elements/BaseField.test.jsx

@@ -1,5 +1,3 @@
-/* eslint-disable no-undef,react/prop-types */


Ah, I see. So you just moved this to a global lint config so you don't need to specify it on each test.

jmiranda · 2018-05-18T14:19:35Z

src/js/tests/__mocks__/styleMock.js

@@ -0,0 +1 @@
+module.exports = {};


Can you explain what this does within the context of mocking CSS files? Does this just clear exports so that nothing is able to contaminate the DOM during tests?

you can check this link for details: https://stackoverflow.com/questions/41841968/react-js-jest-causing-syntaxerror-unexpected-token

jmiranda · 2018-05-18T14:23:31Z

src/js/tests/form-elements/__snapshots__/CheckboxField.test.jsx.snap

@@ -0,0 +1,46 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


Can you explain why snapshots should be treated as code? It seems like it's an artifact of the testing process and should be discarded afterwards?

https://facebook.github.io/jest/docs/en/snapshot-testing.html#1-treat-snapshots-as-code

they should be part of the code, because the test will compare the rendered component with the snapshot and fail if there was any differences

OBPIH-851: Add base form fields components

e5513ff

pmuchowski requested review from jmiranda, kkaczmarczyk and awalkowiak May 17, 2018 12:49

kkaczmarczyk approved these changes May 17, 2018

View reviewed changes

OBPIH-851: Disable 'no-undef' and 'react/prop-types' eslint rules for…

6b7acbd

… tests

awalkowiak approved these changes May 17, 2018

View reviewed changes

kkaczmarczyk mentioned this pull request May 17, 2018

Implement dummy JS API client (wrapper) #279

Merged

jmiranda approved these changes May 18, 2018

View reviewed changes

Add missing form css classes and BaseReactField attributes

6bd0c4d

pmuchowski force-pushed the OBPIH-851 branch from 5fe3591 to 6bd0c4d Compare May 18, 2018 14:42

jmiranda merged commit 7b5fdb6 into develop May 19, 2018

jmiranda deleted the OBPIH-851 branch May 19, 2018 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-851: Add base form fields components #278

OBPIH-851: Add base form fields components #278

pmuchowski commented May 17, 2018

kkaczmarczyk May 17, 2018

jmiranda May 18, 2018

jmiranda May 18, 2018

jmiranda May 18, 2018

kkaczmarczyk May 18, 2018

jmiranda left a comment

jmiranda May 18, 2018

jmiranda May 18, 2018

pmuchowski May 18, 2018

jmiranda May 18, 2018

pmuchowski May 18, 2018

jmiranda May 19, 2018

jmiranda May 18, 2018

jmiranda May 18, 2018

pmuchowski May 18, 2018 •

edited

Loading

jmiranda May 18, 2018

jmiranda May 18, 2018

pmuchowski May 18, 2018

jmiranda May 18, 2018

jmiranda May 18, 2018

pmuchowski May 18, 2018

jmiranda May 19, 2018

jmiranda May 18, 2018

pmuchowski May 18, 2018

		@@ -1,5 +1,3 @@
		/* eslint-disable no-undef,react/prop-types */

OBPIH-851: Add base form fields components #278

OBPIH-851: Add base form fields components #278

Conversation

pmuchowski commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuchowski May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuchowski May 18, 2018 •

edited

Loading