holochain / clutter

Fully distributed twitter built on holochain
161 stars 22 forks source link

Bug set handle modal box #76

Closed science-girl closed 6 years ago

science-girl commented 6 years ago

Added pop up modal box when user first starts clutter. Added profile page where users can edit their first name. 1 first time user pop-up 2x

science-girl commented 6 years ago

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

Connoropolous commented 6 years ago

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

Connoropolous commented 6 years ago

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

Connoropolous commented 6 years ago

@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 commented 6 years ago

I have not used docker and am interested!

zippy commented 6 years ago

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 notifications@github.com wrote:

@science-girl commented on this pull request.

In dna/dna.json https://github.com/holochain/clutter/pull/76#discussion_r189161367:

  • "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 https://github.com/holochain/clutter/pull/76#discussion_r189161367, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAN60BaqU_ehvqxplDg8b89Wvm1agFMks5tzk9PgaJpZM4UAdPi .

science-girl commented 6 years ago

Awesome! Thanks.

Connoropolous commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

Connoropolous commented 6 years ago

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.

science-girl commented 6 years ago

@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 commented 6 years ago

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.