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

Transaction entry #426

Merged
merged 32 commits into from Dec 17, 2016
Merged

Transaction entry #426

merged 32 commits into from Dec 17, 2016

Conversation

aumayr
Copy link
Member

@aumayr aumayr commented Nov 27, 2016

Implementation of #421

  • Basic UI
  • Mobile UI
  • Helpers for UI (autocomplete, formatting, adding new lines, etc.)
  • Data for autocomplete
  • Writing the resulting transaction to the beancount file
  • ~~~Translations (ru, de)~~~ (shouldn't have to be part of feature-PR)
  • Tests
  • Documentation

This is a very big feature to implement, so any help is welcome!

@aumayr aumayr added the feature label Nov 27, 2016
@aumayr
Copy link
Member Author

aumayr commented Dec 1, 2016

There are some issues/tasks still open:

  • Bug: "Save and add new" adds the same transaction multiple times
  • ~~~make indentation configureable~~~
  • dropdown for flag
  • currency as input (awesomplete)?
  • multiple postings
  • ~~~intelligent suggestions for account~~~
  • ~~~tags? links?~~~
  • ~~~metadata?~~~

@@ -32,3 +32,30 @@ def insert_line_in_file(filename, lineno, content):
with open(filename, "w") as file:
contents = "".join(contents)
file.write(contents)


def insert_transaction_in_file(filename, lineno, content):
Copy link
Member

Choose a reason for hiding this comment

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

This should really insert a data.Transaction and use parser.printer for it.

@yagebu
Copy link
Member

yagebu commented Dec 1, 2016

Can you rebase this and fix tests/lint? Then I could help with some of the changes

@aumayr
Copy link
Member Author

aumayr commented Dec 1, 2016

I rebased and fixed the tests/lint.

You can display the transaction form by using the keyboard shortcut n.

Please announce it (here or on Gitter) if you are working on changes, so we do not work on the same.

@yagebu
Copy link
Member

yagebu commented Dec 1, 2016

Seems like you didn't push the rebase, will do that.

Also the autocomplete is currently shown in the wrong position, will fix the css for that and also hide some of the labels, I find placeholders are nicer. This way we can imitate the form looks more like a transaction in the file.

@aumayr
Copy link
Member Author

aumayr commented Dec 1, 2016

I find placeholders are nicer

Yes, this looks much nicer!

@aumayr
Copy link
Member Author

aumayr commented Dec 1, 2016

@yagebu I guess you are adding the currencies-<list> and also still work on the JavaScript (as currencies is now an input, not a select)?

@yagebu
Copy link
Member

yagebu commented Dec 1, 2016

Yes just started with improving the completions a bit.

@aumayr
Copy link
Member Author

aumayr commented Dec 1, 2016

Yes just started with improving the completions a bit.

The algorithm(s) for suggesting the account based on the payee, narration and/or other account are an interesting (and seemingly hard) part. Also, these algorithms could be used (or extended) for implementing a UI for bean-extract, as suggested in #421 (comment)

@yagebu
Copy link
Member

yagebu commented Dec 1, 2016

The algorithm(s) for suggesting the account based on the payee, narration and/or other account are an interesting (and seemingly hard) part. Also, these algorithms could be used (or extended) for implementing a UI for bean-extract, as suggested in #421 (comment)

Right now, I'm only working on the JS part to do fuzzy matchings (and only suggest active accounts). That already goes a long way.

@yagebu
Copy link
Member

yagebu commented Dec 1, 2016

I've added an input for the flag - it's "cheap" as it has a default and value and can simply be skipped. I'm against supporting links, tags and metadata - it doesn't even support costs atm (and I'm fine with that, it's meant for simple transactions IMHO).

<input type="number" step="0.01" class="number" name="number" placeholder="Number" />
<input type="text" class="currency" name="currency" placeholder="Currency" list="currencies" />
<div class="postings">
{% for i in range(15) %}
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a waste of bandwidth. cloneNode should do the trick

@aumayr
Copy link
Member Author

aumayr commented Dec 8, 2016

Should I bake a new release? Doing so would force everyone to use the new
booking code, which is vastly better, but still has a remaining flaw (which
I want to fix soon): https://bitbucket.org/blais/beancount/issues/145

For development we use the current code from bitbucket. I guess when this feature is complete we release a new version, for which we would need a new version/release of beancount as well (for people installing via pip). We will ping you once this is approaching.

@aumayr
Copy link
Member Author

aumayr commented Dec 10, 2016

The currency-input should change it's width for the last row, so it makes space for the "+" and the number inputs are all the same width and neatly vertically aligned.

Currently it looks like this:

screenshot

@yagebu Do you know how to achieve that?

{% for i in range(2) %}
<div class="fieldset posting">
{% for i in range(3) %}
<div class="fieldset posting{% if i == 0 %} template{% endif %}">
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the need for this superfluous element. What was wrong with my code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previous to this change, if you add new posting rows with the "+" the inputs in those rows were not awesomplete-initialized, so there was no auto-complete of them.

Background:
When cloning the .posting-row after pressing "+", the nodes generated by awesomplete were cloned, and the auto-completion did not "carry over" to those nodes, as the list="..."-property got removed by awesomplete.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks for the explanation!

@yagebu
Copy link
Member

yagebu commented Dec 10, 2016

This now uses a (very stupid) handwritten printer that should address most of the concerns above.

I always use two decimal places for EUR. There is no way to configure how many should be added/shown/inserted.

Currently it simply uses whatever the user inputs -- which seems the right thing to do to me.

Currently it looks like this:

That's the look I was going for, I didn't like the tiny currency input. Changing the width's in the conditional last-child code would be the way to change it.

@yagebu
Copy link
Member

yagebu commented Dec 10, 2016

That's the look I was going for, I didn't like the tiny currency input. Changing the width's in the conditional last-child code would be the way to change it.

This should be understood as a "feel free to change it, I don't care much" ;)

@yagebu
Copy link
Member

yagebu commented Dec 10, 2016

The "+"-button and keyboard shortcut are not working when on the Editor page

Can't reproduce. (Of course when the editor is focused the keyboard shortcut doesn't work, but otherwise the shortcut and the button work for me)

@aumayr
Copy link
Member Author

aumayr commented Dec 10, 2016

The "+"-button and keyboard shortcut are not working when on the Editor page

Can't reproduce. (Of course when the editor is focused the keyboard shortcut doesn't work, but otherwise the shortcut and the button work for me)

It works now - sorry for the confusion!

@aumayr
Copy link
Member Author

aumayr commented Dec 14, 2016

There are just two tasks open:

  • When clicking into the results list from awesomplete and the result clicked on is outside the bounds of the overlay, the overlay is dismissed. I tried to fix this, but somehow magically a MouseEvent appears when calling input.close() on the Awesomplete-element. So basically I don't know how to fix this. @yagebu Do you have any clue why this happens?
  • Fix the styling for mobile browsers.

After this is complete, Documentation and Translation have to be added as well.

@yagebu
Copy link
Member

yagebu commented Dec 17, 2016

When clicking into the results list from awesomplete and the result clicked on is outside the bounds of the overlay, the overlay is dismissed. I tried to fix this, but somehow magically a MouseEvent appears when calling input.close() on the Awesomplete-element. So basically I don't know how to fix this. @yagebu Do you have any clue why this happens?

Awesomplete listened to mousedown and we to click (which is basically mousedown+mouseup) which would fire after the list had been closed already.

left: $aside-width;
}
}

.add-transaction-button {
background-color: $color-sidebar-background;
color: $color-journaltable-header;
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 definitely not a variable that should be used here

@@ -584,6 +608,10 @@ td {

&.active {
margin-left: 0;

.add-transaction-button {
left: 160px;
Copy link
Member

Choose a reason for hiding this comment

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

try not to use random constants like this one, we've got variables for this.

height: 41px;
left: 0;
position: fixed;
top: 95px;
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@aumayr
Copy link
Member Author

aumayr commented Dec 17, 2016

@fokusov Could you provide the Russian translation strings?

@fokusov
Copy link
Contributor

fokusov commented Dec 17, 2016

@aumayr Yes, I'm working on it

@fokusov Could you provide the Russian translation strings?

@yagebu yagebu merged commit 8d097c0 into master Dec 17, 2016
@fokusov
Copy link
Contributor

fokusov commented Dec 17, 2016

@aumayr I can't find no new translated strings with make babel-extract in updated templates :(

@aumayr
Copy link
Member Author

aumayr commented Dec 17, 2016

@fokusov Oh, there seem to be no new strings to translate after all 😃 I thought I had translated new parts for German before and the Russian counterparts were missing, but that does not seem to be the case.

Anyhow, thanks for the help!

@yagebu yagebu modified the milestone: 1.2 Dec 25, 2016
@yagebu yagebu deleted the transaction-input-form branch January 19, 2017 18:53
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.

None yet

8 participants