ui-components: integration of modal in native #17

gadjevb · 2018-02-06T08:37:47Z

Integration of a simple modal component in the native part, so it can be later reused in the different apps.

Fixes #16

p-nedelchev · 2018-02-06T08:41:38Z

native/TEST-REACT-NATIVE.xml

-    <testcase classname="Alert Componenent -should render text" name="Alert Componenent -should render text" time="0.139">
+  <testsuite name="/src/Modal/Modal.spec.js" tests="2" errors="0" failures="0" skipped="0" timestamp="2018-02-05T11:27:21" time="0.837">
+    <testcase classname="(Modal) rendering-should render Modal component" name="(Modal) rendering-should render Modal component" time="0.248">
+    </testcase>


Maybe I missed that part but why we push test reporting XML in the repository?

This is strange, git is showing them as ignored.

It was added before to be added to .gitignore. This file should be removed and name of the generated test results file should be improved a bit as TEST-REACT_NATIVE.xml doesn't sound good.

// file with test results of the native package
TEST-RESULTS-NATIVE.xml

mgenov · 2018-02-06T09:56:29Z

native/TEST-REACT-NATIVE.xml

-    <testcase classname="Alert Componenent -should render text" name="Alert Componenent -should render text" time="0.139">
+  <testsuite name="/src/Modal/Modal.spec.js" tests="2" errors="0" failures="0" skipped="0" timestamp="2018-02-05T11:27:21" time="0.837">
+    <testcase classname="(Modal) rendering-should render Modal component" name="(Modal) rendering-should render Modal component" time="0.248">
+    </testcase>


It was added before to be added to .gitignore. This file should be removed and name of the generated test results file should be improved a bit as TEST-REACT_NATIVE.xml doesn't sound good.

// file with test results of the native package
TEST-RESULTS-NATIVE.xml

martinmilev · 2018-02-06T09:57:30Z

native/src/Modal/Modal.js

+    }
+  }
+
+  show() {


open() ?
You should write a story using these functions with "ref".

martinmilev · 2018-02-06T09:57:39Z

native/src/Modal/Modal.js

+    this.setState({ visible: true })
+  }
+
+  close() {


@martinmilev you mean close to become hide or something else you have in mind ?

Yes. I think naming can be improved.

show(), hide() or open(), close()

gadjevb · 2018-02-06T11:48:16Z

Requested changes are made.

p-nedelchev · 2018-02-20T09:45:18Z

Can you consider modal to be opened with a flag like RN Modal there are some cases when you can't use trigger or refs?

mgenov · 2018-02-20T09:50:54Z

Do we have cases with such usages ?

denisdimitrov · 2018-02-20T09:52:46Z

native/src/Modal/Modal.js

+        >
+          <View style={this.props.containerStyles}>
+            <View style={this.props.bodyStyles}>
+              <ScrollView>{children}</ScrollView>


In the body of the modal component in the signer project there is header with cross icon which closes the modal. It's being used for troubles and tasks.
Here is how it looks:

<View style={this.props.containerStyles}> <View style={this.props.bodyStyles}> {this.props.hasCloseHeader && ( <View style={this.props.headerStyles}> <Icon name={'close'} type={'material-community'} onPress={() => this.setState({ visible: false })} /> </View> )} <ScrollView>{children}</ScrollView> </View> </View>

It also has default props:

Modal.defaultProps = { hasCloseHeader: false, headerStyles: { flexDirection: 'row', justifyContent: 'flex-end', margin: 0, padding: 0, borderBottomWidth: 0 } }

Could it be named something easier to remember like "closeButton" and have better styling?

Sure, modify it as you wish.

p-nedelchev · 2018-02-20T10:04:36Z

Actually yes at least when I was migrating activate assets modal there should be opened when a specific error occurred so I can't use a trigger or can't call ref from the reducer. At least that's the way I could solve this so I change the modal in my PR (please have a look https://github.com/clouway/telcong.wss/pull/539/files#diff-674edb0335cb7e016ff6c534ecfa05bdR58)

martinmilev · 2018-02-20T12:48:09Z

return React.cloneElement(child, {
        onPress: () => {
          if (typeof onPressCall === 'function') {
            if (onPressCall.then !== undefined) {
              onPressCall()
                .then(result => {
                  this.props.onShowDialog()
                  return Promise.resolve(result)
                })
                .catch(error => {
                  return Promise.reject(error)
                })
            } else {
              onPressCall()
              this.props.onShowDialog()
            }
          } else {
            this.props.onShowDialog()
          }
        }
      })

gadjevb · 2018-02-20T13:15:47Z

PR is rebased and integrated into storybook. Should opening with flag be implemented?

martinmilev · 2018-02-20T13:17:53Z

Write README.md with description.

martinmilev · 2018-02-20T13:26:40Z

native/src/Modal/Modal.js

+            <View style={this.props.bodyStyles}>
+              {this.props.hasCloseHeader && (
+                <View style={this.props.headerStyles}>
+                  <TouchableOpacity onPress={() => this.setState({ visible: false })}>


this.close() ?

martinmilev · 2018-02-20T13:27:03Z

native/src/Modal/Modal.js

+      if (child.type === Trigger) {
+        return React.cloneElement(child, {
+          onShowDialog: () => {
+            this.setState({ visible: true })


this.show()

gadjevb · 2018-02-20T13:33:22Z

Flag for opening the modal is included.

gadjevb · 2018-02-20T13:55:04Z

I'm not able to hide the StatusBar, I think it's a bug with react native. Here's some more info:
facebook/react-native#7474 - Issue about the problem.
facebook/react-native#18004 - PR for the issue opened 3 days ago.

mgenov · 2018-02-20T13:59:41Z

Ok, as it's just for the example we can live with that for now.

gadjevb · 2018-02-20T14:23:42Z

Ready for a review!

mgenov · 2018-02-20T15:24:34Z

native/src/Modal/Modal.spec.js

+import Modal from './Modal'
+
+describe('(Modal) rendering', () => {
+  it('should render Modal component', () => {


should render Modal component with header.

As I remember there was an issue with shallow/mount rendering and ref statements, but there should be also cases like:

show modal using ref

hide modal using ref

gadjevb · 2018-03-13T09:42:35Z

All requested changes are fixed.

mgenov · 2018-03-13T09:47:53Z

native/src/Modal/README.md

+
+### Usage
+```
+<Modal 


Does this means that the story.js couldn't be used as example ?

The storybook may have a plugin for code example which to be used.

Storybook could be used as an example. We thought that if we include README.md it would provide an additional source of information if someone prefers it.

Yes, but this means that the code in README.md and code in the story book should be maintained at the same time, e.g 2 things to maintain with the same information.

Ok, I will remove the README.md.

martinmilev

LGTM!

Integration of a simple modal component in the native part, so it can be later reused in the different apps. Fixes #16

gadjevb · 2018-03-13T11:29:27Z

@mgenov requested changes are fixed.

mgenov

LGTM !

gadjevb added the needs-review label Feb 6, 2018

gadjevb requested review from mgenov, martinmilev, denisdimitrov and p-nedelchev February 6, 2018 08:37

p-nedelchev reviewed Feb 6, 2018

View reviewed changes

clouway-dev-bot added reviewed and removed needs-review labels Feb 6, 2018

mgenov requested changes Feb 6, 2018

View reviewed changes

martinmilev suggested changes Feb 6, 2018

View reviewed changes

denisdimitrov reviewed Feb 20, 2018

View reviewed changes

martinmilev reviewed Feb 20, 2018

View reviewed changes

mgenov reviewed Feb 20, 2018

View reviewed changes

mgenov reviewed Mar 13, 2018

View reviewed changes

martinmilev approved these changes Mar 13, 2018

View reviewed changes

ui-components: integration of modal in native

94d89b3

Integration of a simple modal component in the native part, so it can be later reused in the different apps. Fixes #16

mgenov approved these changes Mar 13, 2018

View reviewed changes

mgenov merged commit 5996216 into clouway:master Mar 13, 2018

ui-components: integration of modal in native #17

ui-components: integration of modal in native #17

Conversation

gadjevb commented Feb 6, 2018 • edited Loading

p-nedelchev Feb 6, 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

gadjevb commented Feb 6, 2018

p-nedelchev commented Feb 20, 2018

mgenov commented Feb 20, 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

p-nedelchev commented Feb 20, 2018

martinmilev commented Feb 20, 2018

gadjevb commented Feb 20, 2018 • edited Loading

martinmilev commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gadjevb commented Feb 20, 2018

gadjevb commented Feb 20, 2018

mgenov commented Feb 20, 2018 • edited Loading

gadjevb commented Feb 20, 2018

Choose a reason for hiding this comment

gadjevb commented Mar 13, 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

martinmilev left a comment

Choose a reason for hiding this comment

gadjevb commented Mar 13, 2018

mgenov left a comment

Choose a reason for hiding this comment

gadjevb commented Feb 6, 2018 •

edited

Loading

p-nedelchev Feb 6, 2018 •

edited

Loading

gadjevb commented Feb 20, 2018 •

edited

Loading

mgenov commented Feb 20, 2018 •

edited

Loading