New group creation

[hubudibu]

work in progress, will at some point fix #311

[taoeffect]

OK, so, I haven't checked this myself yet, and I could be mistaken, but my gut tells me that:

1. This doesn't need to be a function
2. This doesn't need to be a computed property (have you tried the data: field?)
3. For the stuff inside of it, the get / set stuff of the computed properties is also not necessary

[hubudibu]

1. yes it does, see the api docs: https://vuejs.org/v2/api/#computed also, it's using this inside to use properties defined in data, and this is how that context is bound to the right place

2. and 3.: I did try the data field, which is good for setting an initial value, but what I want to achieve is binding that keeps up-to-date when changed in either the parent or this child component.
   for this I needed to solve how to update form.groupName in the parent when the value in the step child component changes.
   I tried numerous solutions for that:

• firing an event in the step component and manually propagating through 4 components to get here 🤾‍♀️🤾‍♂️🤾‍♀️🤾‍♂️, and
• creating createGroup's own event bus for a more direct communication between the step and the parent 🚎
  that you can see in the previous commits.
  both those ways seemed way more cumbersome than this solution, where I directly use the computed value's setter to update data in the parent component.
  And you can't use getter/setters for data, only for computed props.

I'm still thinking about creating a little tool that automates this kind of binding and makes it less ugly (I miss React's HOCs), but apart from that, this is the simplest solution I could find. let me know if you still see ways to simplify!

[taoeffect]

yes it does, see the api docs: https://vuejs.org/v2/api/#computed also, it's using this inside to use properties defined in data, and this is how that context is bound to the right place

Ah you're right, I think I might have been just used to seeing it written as views () instead of views: function (), and subconsciously forgotten those are the same thing, lol.

OK, this looks good, I will still play with it and see if I can think of any other way, but I'm getting more convinced that you've found the correct approach.

Kudos on the connect function, and nice job with making this PR conform to #297. 👍

[taoeffect]

What do you think about this: instead of having the "change" link turn the textfield into an input field, have it reveal a slider below the numbers that the user can click and drag to adjust the number?

[taoeffect]

Also: I'm reminded by this quote from our post on Deprecating Majority Rule:

Therefore, it is dangerous to use majority rule whenever supermajority rule should be used instead, e.g. passing laws. That is why we plan on making supermajority the default threshold for removing members in Group Income, since doing so can have a potentially significant impact on a person’s life.

We should make it so that a function is called when the slider is dragged, and when it's dragged to a "bad place", Group Income will be able to make a warning field appear that explains to them that they're choosing a value that is most likely too low, and even linking them to our blog posts, etc.

[taoeffect]

The font here is too big for the group purpose to be displayed (which can be quite long potentially).

Also, don't forget that the error text here needs to be localized (see comment above).

Honestly, I think that, as per the comments elsehwere, we shouldn't even have the option of saying 'No group purpose set', it should just always be set by the time this view is displayed, IMO.

[hubudibu]

@taoeffect I tried out vuelidate for validation, if you agree that this is a good direction then I will go through the codebase and replace all existing validation with this as of #334

About validation, the only thing remaining is an enter guard for routes of steps after an invalid one, let's talk at today's call whether it's necessary.

I addressed most of the comments too, the only things I see remaining are

• currency unit selector on minimum income step and
• rule change slider with info notification on 'majority vote' when too low - I love this idea!

For the design questions, I would consider them low priority until we have progress on #327, I assume lot will change here too when that one moves forward. Let me know if you see something that damages usability and should be still done.

Happy New Year! 🥂

[taoeffect]

@hubudibu Slack seems to be having some down time today, but I posted a reply yesterday (a little late in the day).

Basically, yeah, I love your idea to use the circle slider around the numbers, that sounds really fun and cool! 😄

And if you want to chat about anything, you can always send me a DM on twitter, or post to the gitter, which thankfully is still up: https://gitter.im/okTurtles/group-income

[hubudibu]

new slider circles!
I removed the 'change' button solely because it looks cool to have the circles always showing. however, I'm not sure the interaction needed to change the values - just grab the knob - is obvious for every user this way. @taoeffect let me know what you think about this. it would be great to user-test it :)

now my only problem is that the rule setting step is not tested, and neither is the circle slider package 😱 I'm trying to come up with a solution.

in the meantime, looking forward to your review @taoeffect !

[taoeffect]

I have a few concerns about this cyy component:

1. It doesn't really do much, so it seems unnecessary (plus its name is unclear)
2. Because it doesn't use bindings, it's not useful in situations where the input currency changes
3. As far as I can tell, it would probably be much simpler to just use a computed property that passes group.incomeCurrency through the symbol function instead of a <cyy> component

[hubudibu]

you are right, they don't make much sense like this. I had some confusion about how the slots are supposed to work, and just copied some of our i18n component's workings here, but now that I checked it again, yes, the beforeMount in cyy of course won't run again and update the value when it changes in the slot.
changing to computed properties.

[taoeffect]

This v-if/v-else condition shouldn't be necessary, right? By this point the validations should guarantee that group.groupName is set?

[hubudibu]

well, as I mentioned, all steps having their own URL makes it pretty easy to end up at this step, even with a simple refresh, that's why I kept these. is that something we should worry about?

[taoeffect]

Hmm, technically you are correct, if the user is using the app in a browser, they could visit this page somehow... without setting the groupName... theoretically, perhaps by looking at the browsing history.

I have two suggestions on how to deal with this:

1. Just make sure that vuelidate doesn't allow the form to be submitted (I'm guessing it already does that? Can you verify?)
2. Have a guard of some sort, perhaps in mounted (), i.e. maybe a mixin that checks the variables that need to be set by this step, and if they aren't set revert to the beginning of the StepAssistant.

EDIT: OK, I see that (1) above is the behavior. I guess that is fine. :)

[hubudibu]

1. yes, that's the current behavior :)

2. here are my two suggestions:

2.1 the full solution would be to check for every step that all the previous steps are valid, and if not, go back to the first invalid step. this seems like a bigger task, but we have everything for this to work - the step-validator relation and the order of steps
2.2 the quicker solution would be to only guard the summary step, and if it's not valid, jump back to the first step of the assistant. this could work if we assumed that the only way to end up here in an invalid state is by skipping all the previous steps. but it's not the case, someone could easily hit refresh at the 2nd step and only miss the first one. from the user's perspective, without a proper error notification solution #175 , I think this is way more confusing and frustrating than seeing on the summary which step is actually missing and having to manually go back to correct it.

so in my opinion, what we have now is better than 2.2, and I'd love to do 2.1, but I assume we have things with higher priority.
thoughts? :)

[taoeffect]

Same concern here as above.

[taoeffect]

Same as above + other comment below about <cyy> component.

[taoeffect]

Should be "No one invited" I think.

[taoeffect]

Nice, but remember to put all text, including this, into <i18n> tags. ^_^

[hubudibu]

shoot!