Skip to content
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

Feature/settings #27

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Feature/settings #27

wants to merge 21 commits into from

Conversation

m4gpi
Copy link
Member

@m4gpi m4gpi commented Feb 14, 2019

Integrating a profile editor, provides:

  • Add a form
  • Add avatar field (with image cropper)
  • Add name field
  • publish to log
  • write query to calculate number of about messages
  • explore using $reduce on the query
  • render prompt on homescreen leading person to publish a name for identification
  • show local peers
  • style local peers
  • refactor and lint

Also added some other things...

  • removed the 'show' page and used a view component as a modal instead to standardise with the rest of the application.

@m4gpi m4gpi added the WIP label Feb 14, 2019
}
}
},
// %%TODO%%: Work out why reduce isn't working?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mixmix any idea why reduce wouldn't be working here? Trying to just fo a count of the number of about messages. To be honest, what I really want is to grab the first about message, and terminate the stream if we find one. This is to be able to show a warning to the user to notify them that they haven't added any personal information yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KGibb8 I'm gonna skip to your question about what you'd like to query (because don't know about reduce easily)

  • you can say limit: 1 and it will quite out after first match is found
  • you might also want to do reverse: true because that will give you the most recent about message
  • not sure, you might also want to add some filters on the about fields that are present ... e.g. a name which is a string of length > 2 ? you might need to not use limit: 1 and instead use a drain and a http://pull-stream.github.io/#pull-abortable to cancel once you've confirmed some things a little more carefully in the drain

@m4gpi m4gpi requested review from mixmix and ameba23 February 21, 2019 15:41
@m4gpi
Copy link
Member Author

m4gpi commented Feb 21, 2019

top:50%
margin-left:8px
transform: translateX(0%) translateY(-50%)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 this is editing the global state, not data tooltips inside Tooltip. Needs to be changed somehow

in case it's helpful you can do:

Tooltip {
  (div) {
    [data-tooltip-position='right']::after{ .... }
  }
}

this will make a rule like :

// css
div.Tooltip div[data-tooltip-position='right']::after { .... }

note the difference with the () not around div

Tooltip {
  div {
    [data-tooltip-position='right']::after{ .... }
  }
}

this will make a rule like :

// css
div.Tooltip > div[data-tooltip-position='right']::after { .... }

much more rigid ... > means "direct child only"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually using this code anywhere at the moment so we don't need it. I didn't realise it was still there!!

@mixmix
Copy link
Member

mixmix commented Mar 12, 2019

hey @KGibb8 this is a lot of changes in one PR, which means I haven't had time to load it all into my head and review inside the timebox I set aside.

Is it possible for you to seperate this into different pull-requests:

  • the profile editing stuff
  • show local peers
  • the refactoring of other views / modals

The other reason I'm keen on the profile editing stuff being in a seperate place is that I think I'm going to need a copy of this upstream in ssb-ahoy - as in, when we're managing the onboarding upstream there I want them to be able to be setting their name / image and syncing from one another locally.

We're still going to need profile editing and local peers in the main app as well of course. We probably need to talk about this Soon

@m4gpi
Copy link
Member Author

m4gpi commented Mar 13, 2019

With regards to splitting this up, owch thats gonna be painful and time consuming, particularly the modals (as both the new views use the modals so splitting the CSS up is gonna take time...). I'll assess how long thats gonna take... before I invest too much time in that, it makes more sense for me to know about your ssb-ahoy plans and how it affect this.

So having written made quite a few changes in development branch of dark-crystal-standalone, and having a clearer idea of what that looks and functions like (it no longer includes this module or Patchbay for that matter), I'm feeling less opinionated about where the profile editing and network code should go, if this is a feature that patchbay needs / wants at some point, would you rather this were a separate patchbay module? If so, and it isn't going into patchbay-dark-crystal, then the main parts of this PR that deserve a merge are the CSS and modal refactor. The rest of the code can probably be used elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants