Contributions page creation #480

sandrina-p · 2018-09-03T07:49:15Z

Desktop

Tablet

Add / edit contribution flow

What's done:

Hardcoded data with 3 giving contributions for testing purposes
UI Logic to add/edit contributions with hardcoded submit for testing purposes.

Cool features

Add / Delete contributions by using cursor or pressing Enter
Cancel from adding/editing contribution by clicking "Cancel" or pressing Esc
The placeholder to add a new contribution is a random value from 5 predefined placeholders 🎲
All the actions can be achieved by just using screen readers with meaning names (labels and errors).

Next step

Since this PR is already big I suggest to deal with Monetary contributions UI Logic in a separate branch/PR. That way this one is easier/faster to review/merge.

Girl scout Rule

Fix #481

frontend/simple/views/components/Lists/ListItem.vue

…older

frontend/simple/views/Contributions.vue

frontend/simple/views/components/Contribution/Contribution.vue

sandrina-p · 2018-09-05T21:47:55Z

frontend/simple/views/components/Contribution/Contribution.vue

+  },
+  methods: {
+    handleClick () {
+      if (this.$listeners.click) {


Note: I don't like very much this condition, but it was the cleaner way I've found to conditionally trigger a different behaviour when the contribution is monetary.
Maybe a prop isMonetary is more intuitive...

I'm not sure about this this.$listeners approach... it seems very confusing and arbitrary to me.

With this approach, there is no clear relationship between the parent component and this one. It's not clear semantically what it means for this.$listeners.click to exist, so I would not rely on it for semantic meaning as is being done here.

Instead, I would recommend using $emit to emit events that the parent component can listen for and act appropriately.

Okay, I'll use the $emit like I did with @new-value, actually it's way obvious.

But nevertheless it's needed to confirm if the parent is listening to the $emit. When it's not, it should set this.isAdding to true (that shows the input)

sandrina-p · 2018-09-05T21:54:53Z

frontend/simple/views/components/i18n.vue

@@ -12,7 +12,7 @@ export default {
      default: 'span'
    }
  },
-  computed: {
+  methods: {


(I've already wrote this, but the comment disappeared, so I'm writing it again, sorry if you get a double notification)

@taoeffect there was a weird bug on input save/delete buttons when using i18n.

Note how the delete button (red color) has the Save button text "save" and not "delete". The DOM/JS is correct: The delete button (red save), deletes the contribution and the save button (blue save), updates the text.

This was really tricky and I took a while to understand the problem was from i18n and not the component itself. Changing translatedText from computed to methods seemed to fix the issue and each button has the correct text, but honestly I can't explain exactly why that bug happened...

taoeffect · 2018-09-05T22:44:41Z

frontend/simple/views/Contributions.vue

+}
+</style>
+<script>
+import { symbol } from './utils/currencies.js'


If you want to "boy/girl scout rule" this one, I just opened a minor issue: #481

You don't need to do it for this PR, only if you want. It would require doing some replacements in other files as well.

I'll do it later :)

taoeffect · 2018-09-05T23:18:47Z

frontend/simple/views/Contributions.vue

+            variant="editable"
+            @click="showGivingMincomeSettings"
+          >
+            <span class="has-text-weight-bold">{{currency}}{{giving.monetary}}</span> <i18n>to other's mincome</i18n>


Some languages may reverse the order of the information... so it might be better to place the span inside the i18n...

taoeffect · 2018-09-05T23:26:00Z

frontend/simple/views/Contributions.vue

+          >
+            <strong v-if="isMonetary(contribution.type)">{{textGivingMonetary(index)}}</strong>
+            <strong v-else>{{contribution.what}}</strong>
+            <i18n>from</i18n>


Similar to the comment below, the problem with chopping up strings like this (outside of <i18n> tags), is that some languages may reverse the order of words.

In other words, here we have a sentence that looks like this:

Cooking from Lilia Bouvet

But another language might write that like so:

Bouvet Lilia Cooking from

Or something else entirely crazy.

This is why you need to go through the entire PR and look at all areas like this, and ensure that all sentences are enclosed entirely within a single <i18n> tag, and use the :args= binding to pass in variables.

It makes sense, I'll look into that later.

Here we're dealing with text within html tags and part of it has a button that is a tooltip... Kinda tricky. @taoeffect how do you suggest to implement the translation here?

frontend/simple/views/components/Contribution/Contribution.vue

taoeffect · 2018-09-05T23:45:10Z

frontend/simple/views/components/Contribution/Contribution.vue

+      return this.placeholders[Math.floor(Math.random() * this.placeholders.length)]
+    },
+    iconClass () {
+      return this.$listeners.click ? 'fa-ellipsis-v' : 'fa-edit'


(Read this comment after the one below about this.$listeners)

Similarly, here I would rely on some internal this.state variable that has a value of possible enums (defined as constants at the top of this file, perhaps)

possible enums? like what? check what I've done to see if it's ok...

…utton to input

taoeffect · 2018-09-06T22:52:08Z

frontend/simple/views/Contributions.vue

+          <contribution
+            v-for="contribution, index in receiving"
+            :variant="isMonetary(contribution.type) ? 'editable' : undefined"
+            v-on="isMonetary(contribution.type) ? { 'control-click': showReceivingSettings } : {}"


We need to avoid the use of 'control-click' for a couple reasons: (1) it can be confusing for some users, especially Mac users, who are not used to control-clicking things, and (2) it doesn't exist on mobile

control-click doesn't have nothing to do with ctrl key... it means the dev controls the click on the contribution and doesn't trigger the input, yah... horrible name. I'll find a better name and let you know about it

taoeffect · 2018-09-06T23:03:04Z

frontend/simple/views/Contributions.vue

+            >{currency}{amount} for mincome</i18n>
+            <strong v-else>{{contribution.what}}</strong>
+            <i18n>from</i18n>
+            {{getWho(contribution.who)}}


So is there a reason that lines 23, 24, and 25 can't be placed into the i18n tag on line 20? Remember that at least entire sentences must be translated as a whole, not as parts, as otherwise it might be impossible to translate the UI into some language

Maybe you didn't see my last comment yesterday asking for you advice on the implementation for this exact lines:

Here we're dealing with text within html tags and part of it has a button that is a tooltip... Kinda tricky. @taoeffect how do you suggest to implement the translation here?

So, instead of having Cooking from John we'll have <\strong>Cooking</strong> from John.

Actually I've found a way using Vue native directive v-html Raw-HTML... but we still have the button/tooltip (and X others part) out of i18n... check the last commit and let me know your thoughts.

taoeffect · 2018-09-06T23:09:55Z

frontend/simple/views/components/Contribution.vue

+      return this.placeholders[Math.floor(Math.random() * this.placeholders.length)]
+    },
+    iconClass () {
+      return this.hasControlClickListener ? 'fa-ellipsis-v' : 'fa-edit'


possible enums? like what? check what I've done to see if it's ok...

Yeah I'm saying that checking whether there is a click listener, ctrl-click listener, etc, is not semantically meaningful. In other words, it's not clear at all what that means, when what you're trying to say here is that "this contribution is of a certain type". That is not conveyed at all by assigned a control-click listener.

For example, the control-click listener needs to be removed anyway (see the comment above about that), but that shouldn't have any impact on what goes on here.

So what you want to do is pass in a param to the component that sets its type (or, if it's not about type, but about what state the contribution is in, then it would set the initial this.state variable to some value).

Then either the this.type or this.state would be checked here. And it needs to be semantically clear what you're checking, what it means in other words. Like well written English. this.hasControlClickListener tells me nothing about what is being checked here and why 'fa-ellipsis-v' is returned in one case, and 'fa-edit' is returned in another. The code should read like a book and tell me that.

I completely agree with you and that was the comment I had before, with a TODO about my struggle to find the right name to reflect the context correctly.

So... let me put it into words and maybe find a good name for it. The component <contribution>'s state can be "self-controllable" or not:

When it is, it shows the input to create/edit a contribution after the user interacts with it - the edit icon button or all the contribution (ex: Add non-monetary method)). This happens on non-monetary contributions.

When not, instead of showing the input, the parent is the one who decides what happens when the user interacts with it. In this case, to show (in a next PR) a Modal with Monerary methods. This happens on monetary contributions.

After this... maybe the best prop name is isMonetary and the best custom-event name is @interact.

In practice..
if isMonetary it has the icon 'fa-ellipsis-v', otherwise it has 'fa-edit'.
if isMonetary, on click it emits @interact, otherwise it shows the input to edit the contribution.

Does it makes more sense now?

taoeffect · 2018-09-06T23:10:44Z

frontend/simple/views/components/Contribution.vue

+  methods: {
+    handleClick () {
+      if (this.hasControlClickListener) {
+        this.$emit('control-click')


This needs to be a different event that's semantically meaningful. If after reading all my comments here it's still not clear what I mean by that, then let me know and we can do a video chat about it :)

taoeffect · 2018-09-07T23:20:20Z

frontend/simple/views/Contributions.vue

+            :isMonetary="isMonetary(contribution.type)"
+            @interaction="showReceivingSettings"
+          >
+            <span v-html="textReceivingContribution(contribution, index)"></span>


This seems acceptable for now, and it seems like a flexible enough approach that we can modify it if necessary for the section containing the tooltip below. Nice job! 👍

sandrina-p added 5 commits September 2, 2018 14:14

Contributions - Mount basic ldefault layout

0f5a5bc

Contributions - Add nonmonetary method input

4b5b900

Contributions - Refine list of contributions

5bcd9b3

Contributions - Style input to add nonmonetary

d1eee38

Contributions -NonMonetary ui logic

ac7e267

sandrina-p commented Sep 3, 2018

View reviewed changes

frontend/simple/views/components/Lists/ListItem.vue Outdated Show resolved Hide resolved

sandrina-p added 2 commits September 4, 2018 13:15

revert ListItem.vue

4d47ccd

Contributions - start DRYing contribution component

b6e88ac

sandrina-p force-pushed the feat/contributions-page-creation branch from 6f587c2 to 70b5b6a Compare September 5, 2018 20:16

Contributions - extract Contribution UI logic from view

6486b42

sandrina-p force-pushed the feat/contributions-page-creation branch from 70b5b6a to 6486b42 Compare September 5, 2018 20:18

sandrina-p requested a review from taoeffect September 5, 2018 21:33

sandrina-p assigned taoeffect Sep 5, 2018

sandrina-p changed the title ~~WIP: Contributions page creation~~ Contributions page creation Sep 5, 2018

Contributions - Add some comments, adjust pixels and randomize placeh…

39a5045

…older

sandrina-p commented Sep 5, 2018

View reviewed changes

frontend/simple/views/Contributions.vue Outdated Show resolved Hide resolved

sandrina-p commented Sep 5, 2018

View reviewed changes

frontend/simple/views/Contributions.vue Outdated Show resolved Hide resolved

sandrina-p commented Sep 5, 2018

View reviewed changes

frontend/simple/views/components/Contribution/Contribution.vue Outdated Show resolved Hide resolved

sandrina-p commented Sep 5, 2018

View reviewed changes

taoeffect reviewed Sep 5, 2018

View reviewed changes

frontend/simple/views/components/Contribution/Contribution.vue Outdated Show resolved Hide resolved

taoeffect reviewed Sep 5, 2018

View reviewed changes

Contributions - updates based on code review

bac8a22

sandrina-p force-pushed the feat/contributions-page-creation branch from 5f95937 to bac8a22 Compare September 6, 2018 06:54

sandrina-p added 3 commits September 6, 2018 14:01

Contributions - updates based on code review

82b03dc

Contributions - fix bug on contribution with keyboard enter on edit b…

56dd0c8

…utton to input

fix okTurtles#481 improve currencies.js

b8e94da

taoeffect reviewed Sep 6, 2018

View reviewed changes

sandrina-p added 2 commits September 7, 2018 13:38

Contributions - improve props/conditions readability

a1b9b40

Contributions - refactor i18n to treceiving contribution text

66c9a44

taoeffect reviewed Sep 7, 2018

View reviewed changes

taoeffect merged commit 2950dab into okTurtles:master Sep 7, 2018

sandrina-p mentioned this pull request Nov 10, 2018

Implement the "3 questions" during onboarding #483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contributions page creation #480

Contributions page creation #480

sandrina-p commented Sep 3, 2018 •

edited

sandrina-p Sep 5, 2018

taoeffect Sep 5, 2018

sandrina-p Sep 6, 2018

sandrina-p Sep 6, 2018

sandrina-p Sep 5, 2018 •

edited

taoeffect Sep 5, 2018

sandrina-p Sep 6, 2018

taoeffect Sep 5, 2018

taoeffect Sep 5, 2018

sandrina-p Sep 6, 2018

sandrina-p Sep 6, 2018

taoeffect Sep 5, 2018

sandrina-p Sep 6, 2018

taoeffect Sep 6, 2018

sandrina-p Sep 7, 2018

taoeffect Sep 6, 2018

sandrina-p Sep 7, 2018

sandrina-p Sep 7, 2018

taoeffect Sep 6, 2018

sandrina-p Sep 7, 2018

taoeffect Sep 6, 2018 •

edited

taoeffect Sep 7, 2018

Contributions page creation #480

Contributions page creation #480

Conversation

sandrina-p commented Sep 3, 2018 • edited

Desktop

Tablet

Add / edit contribution flow

What's done:

Cool features

Next step

Girl scout Rule

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

sandrina-p Sep 5, 2018 • edited

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

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

taoeffect Sep 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandrina-p commented Sep 3, 2018 •

edited

sandrina-p Sep 5, 2018 •

edited

taoeffect Sep 6, 2018 •

edited