Bug set handle modal box #76

science-girl · 2018-05-16T00:11:15Z

Added pop up modal box when user first starts clutter.
Added profile page where users can edit their first name.

…nuing

science-girl · 2018-05-16T00:15:11Z

Did Travis fail because it's using old flags ie. no-nat-pnp?

Connoropolous · 2018-05-16T19:25:03Z

@science-girl quite possibly. I will get reviewing this now

Connoropolous

Overall sweetness. The pattern used with this.props.children on the modal is especially good because it sets up something that's easily extendable.

A couple of overall suggestions:

Best practice would be to make sure that the UI handles more errors as well, and displays error messages.
When submitting pull requests, its useful to separate stylistic pull requests, (all the linting you did, which is great!) from functionality pull requests, so that code reviewers can focus on function, and the real changed files.
In the case of the 'profile photo' addition, it's also good to submit code in pull requests as more complete features. Introducing partial code for features gets unwieldy fast. Easy to lose track of what's actually working from what's not yet working.

Connoropolous · 2018-05-16T20:04:25Z

dna/clutter/clutter.js

-    if (name == "App_DNA_Hash")   {return   App.DNA.Hash;}
-    return "Error: No App Property with name: " + name;
+  } catch (exception) {
+    debug(exception);


Best practice would be to return an error if an error occurs

Connoropolous · 2018-05-16T20:07:13Z

dna/clutter/clutter.js

+  } catch (exception) {
+    debug(exception);
+  }
+  return data;


In the function comments, it says it will return the hash, but it returns the raw input instead. Best to reconcile them, one way or another.

Connoropolous · 2018-05-16T20:07:44Z

dna/clutter/clutter.js

+ * @param none
+ * @return firstName associated with this user
+ **/
+function getFirstName(then) {


Unused param in the input here

Connoropolous · 2018-05-16T20:16:12Z

ui-src/src/EditProfile.js

+    e.preventDefault();
+
+    if (!newNameText) return;
+    setFirstName(newNameText);


Once the action/request completes, the user expects some feedback.
This could be done with some kind of transient user feedback message like 'Name was saved' that comes into the page, then out. Could also be done with a redirect away from the profile page.

I will review how to go about treating an action as a Promise so that you can perform some code once the action completes.

done - goes back to meow container. also added first name set to handle as a default.

Connoropolous · 2018-05-16T20:17:34Z

ui-src/src/EditProfile.js

+  }
+  componentDidMount() {
+    const { firstName } = this.props;
+    this.setState({ newNameText: firstName });


Nice pattern to use

Connoropolous · 2018-05-16T20:21:55Z

ui-src/src/Settings.js

+      <div className="panel panel-default">
+        <div className="panel-body">
+          <div style={{ paddingLeft: 30, paddingBottom: 10 }}>
+            <p


Another way to toggle visibility of elements based on boolean, as an option

{this.state.newHandleText.length === 0 && this.props.handleTaken === false && <p>...</p>}

Leaving it as is.

Connoropolous · 2018-05-16T20:26:38Z

ui-src/src/actions/index.js

+export const GET_AGENT = "getAgent";
+export const NEW_HANDLE = "newHandle";
+export const UNFOLLOW = "unfollow";
+export const TOGGLE_MODAL = "toggleModal";


TOGGLE_MODAL could be separated out from the "Holochain actions", since it's a UI only action, and has no backend implications. Just by putting it above the "Holochain actions" comment above, and giving it a "UI actions" comment or something

Connoropolous · 2018-05-16T20:28:54Z

ui-src/src/components/Modal.js

+  render() {
+    // Render nothing if the "show" prop is false
+    if (!this.props.show) {
+      console.log(this.props);


Good to remove debugging console.log statements

Connoropolous · 2018-05-16T20:33:28Z

ui-src/src/reducers/index.js

  appProperties: {},
  // posts with 'stamp' as their key
  posts: {},
+  isOpen: true,


using a slightly more descriptive var name is a bit better. What isOpen? modalIsOpen

Connoropolous · 2018-05-17T10:31:36Z

Got the command line flags passing...
Now we need to fix the tests that run in the ui-automation folder

… review

Connoropolous · 2018-05-17T20:34:25Z

@science-girl have you used docker? If so, I can clarify how to use docker to run the ui-automation tests. Or even if you haven't used it, but are interested I can show you. It is pretty neat (and useful) to see the Cypress tests that Philip wrote running (because you can actually visually see it testing the app).

science-girl · 2018-05-17T21:21:03Z

I have not used docker and am interested!

…clutter into bug-setHandleModalBox

lucksus · 2018-05-18T00:02:46Z

dna/dna.json

+  },
+  "BasedOn": {
+    "H": null
+  },


With the latest head in https://github.com/holochain/holochain-proto develop branch, this
"BasedOn": { "H": null }
makes it fail.

Would you be able to provide more background on this? I don't know what purpose "BasedOn" serves, which I think would help with fixing it.

lucksus

Nice work in general! :)

I've tried setting the name in your new EditProfile component. The name gets stored in the front-ends state but does not seem to be written to holochain - at least it is gone when I reload the page.

lucksus · 2018-05-18T00:19:33Z

ui-src/src/App.js

-  componentWillUnmount () {
-    if (this.interval) clearInterval(this.interval)
+  componentWillUnmount() {
+    if (this.interval) clearInterval(this.interval);


Not sure if you set a styleguide that requires semicolons, but JS does not (any more).
(And I personally would prefer not having them but ok :D)

I prefer semi-colons, but I can omit them for consistency. Is there a style guide I should follow?

I think you're right about the refresh - it's set, but I need to call getFirstName to ensure I retrieve the firstName from the chain rather than just from the redux store. I'll look into that.

zippy · 2018-05-18T04:52:51Z

Based on is a tag that we added to allow you to indicate which DNA this DNA came from. The format of the storing of hashes changed from an object to a string, so you can fix this by simply removing the whole based on key when null, it's not necessary.

…

On Fri, May 18, 2018, 12:34 AM Lindsey Woo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dna/dna.json <#76 (comment)>: > + "Version": 0, + "UUID": "00000000-0000-0000-0000-000000000000", + "Name": "clutter", + "RequiresVersion": 21, + "Properties": { + "description": "distributed micro-blogging (Twitter clone)", + "language": "en", + "enableDirectoryAccess": "true" + }, + "PropertiesSchemaFile": "properties_schema.json", + "DHTConfig": { + "HashType": "sha2-256" + }, + "BasedOn": { + "H": null + }, Can you provide more background on this? I don't know what purpose "BasedOn" serves and how I can fix this. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#76 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAN60BaqU_ehvqxplDg8b89Wvm1agFMks5tzk9PgaJpZM4UAdPi> .

science-girl · 2018-05-18T05:15:20Z

Awesome! Thanks.

Connoropolous · 2018-05-18T17:50:47Z

@science-girl maybe leave the 'profile photo' field in the code, but comment it out so its not in the UI yet while its an incomplete feature?

Connoropolous · 2018-05-18T17:58:10Z

@philipbeadle is much stronger with docker than me

but to run the cypress tests in a docker way

  # build the ui-src files into the ui folder, docker instances run off this built UI folder.
  cd ui-src
  npm install
  npm run build
  cd ..
  # spin up the three separate docker instances
  TARGETDIR=$(pwd) docker-compose up
  # run the delightful UI tests with cypress that rely on the docker instances
  cd ui-automation
  npm install
  npm run cypress:docker

Connoropolous · 2018-05-18T18:01:33Z

@science-girl first name thing wasn't working because you were using the setState pattern in componentDidMount but getFirstName was an asynchronous action. I switched to

componentDidUpdate(prevProps) {
    const { firstName } = this.props
    if (!prevProps.firstName && firstName) {
      this.setState({ newNameText: firstName })
    }
  }

It's necessary to use the conditional like this, otherwise you end up in infinitely recurring loops caused by updating the component, and then updating the component when the component updates. Gotta be real careful with these ones.

Connoropolous · 2018-05-18T18:08:42Z

@science-girl and the pre-requisite that I forgot to mention for using docker is of course installing it :p that's pretty straightforward
https://www.docker.com/community-edition#/download

science-girl · 2018-05-18T19:01:18Z

Thanks @Connoropolous! I rarely use componentDidUpdate - is this preferred to async/awaiting?

Connoropolous · 2018-05-18T20:27:18Z

Not better or worse, just different. I think I may still have a small issue with my async (then) pattern with hc-redux-middleware so that's why I went this way.

…e firstName is replacement

science-girl · 2018-05-20T21:51:52Z

@Connoropolous I ran the docker tests. 2 tests fail - there's a test for changing the handle, which no longer applies. I'll connect with @philipbeadle about fixing this.

lucksus · 2018-05-22T20:48:26Z

Ok, now the name gets persisted across reloads :)

But, I saw something else: on first use (first time loading the UI) I saw form widgets for adding a user image. After reload those elements were gone.

lucksus

Merge at will :)

science-girl added 6 commits May 10, 2018 14:00

added modal and logic for insuring a handle has been set before conti…

f43213e

…nuing

added styles to modal box

e7cd789

added re-setting name functionality

ad16070

name field populated from redux store upon profile load

2d9060a

Merge branch 'develop' into bug-setHandleModalBox

24ad8db

removed unused vars

5b65800

science-girl self-assigned this May 16, 2018

science-girl added in progress review and removed in progress labels May 16, 2018

science-girl requested a review from Connoropolous May 16, 2018 00:13

Connoropolous reviewed May 16, 2018

View reviewed changes

Connoropolous added 2 commits May 16, 2018 16:48

fix docker tests for holochain latest

debc55d

also fix test flags in .travis.yml

cc1a8d8

science-girl added 2 commits May 17, 2018 11:53

incorporated backend error handling and visibility toggling from code…

1bbb710

… review

changed visibility toggling back because the whole panel disappeared

2700d60

science-girl added 2 commits May 17, 2018 14:31

added check if first name has not been changed then dont update

eaceb93

Merge branch 'bug-setHandleModalBox' of https://github.com/Holochain/…

c545588

…clutter into bug-setHandleModalBox

lucksus suggested changes May 18, 2018

View reviewed changes

science-girl mentioned this pull request May 18, 2018

Bug set handle modal box #75

Closed

science-girl added 2 commits May 17, 2018 22:18

deleted BasedOn in DNA

c63020c

removed semi-colons

c0e847a

science-girl and others added 2 commits May 18, 2018 10:36

debugging firstName on refresh

cd3ab69

fixed firstName not displaying bug

bba5228

fixed more bugs related to firstName

912e030

delete hash and link when updating firstName to prevent errors if sam…

2f72e98

…e firstName is replacement

science-girl added 10 commits May 21, 2018 14:03

changed ui-automation tests to test new functionality

9c86c05

edited ui automation tests to account for changes with modal box

d4b880b

removed semi-colons

3791cbe

removed H from post.json test 10

e581f66

removed H ref from posts

cbc7ee8

testing tests by commenting out offending part of ui-test

6376e26

testing tests by commenting out offending part of ui-test

30320ab

testing tests by hard-coding hash for test 10

90cf705

added max length check for handle

fc8ea83

errant console log statement

6ffa32e

lucksus approved these changes May 22, 2018

View reviewed changes

science-girl added 2 commits May 22, 2018 14:17

fixed merge conflict

09c3f00

fixed merge conflict

a749f72

science-girl merged commit fcc5d54 into develop May 22, 2018

science-girl removed the review label May 22, 2018

science-girl deleted the bug-setHandleModalBox branch May 24, 2018 00:12

Bug set handle modal box #76

Bug set handle modal box #76

Uh oh!

Conversation

science-girl commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

science-girl commented May 16, 2018

Uh oh!

Connoropolous commented May 16, 2018

Uh oh!

Connoropolous left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

science-girl May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Connoropolous commented May 17, 2018

Uh oh!

Connoropolous commented May 17, 2018

Uh oh!

science-girl commented May 17, 2018

Uh oh!

lucksus May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

science-girl May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucksus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

science-girl May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

science-girl May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zippy commented May 18, 2018 via email

science-girl commented May 16, 2018 •

edited

Loading

science-girl May 17, 2018 •

edited

Loading

lucksus May 18, 2018 •

edited

Loading

science-girl May 18, 2018 •

edited

Loading

science-girl May 18, 2018 •

edited

Loading

science-girl May 18, 2018 •

edited

Loading

Connoropolous commented May 18, 2018 •

edited

Loading

science-girl commented May 18, 2018 •

edited

Loading