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

Implement exporters config ui #107

Merged
merged 33 commits into from
Oct 7, 2020
Merged

Implement exporters config ui #107

merged 33 commits into from
Oct 7, 2020

Conversation

brafdlog
Copy link
Owner

@brafdlog brafdlog commented Sep 21, 2020

Resolves #83

Arielgordon123 and others added 22 commits August 19, 2020 07:48
# Conflicts:
#	src/originalBudgetTrackingApp/configManager/configManager.ts
#	src/originalBudgetTrackingApp/configManager/defaultConfig.ts
#	src/originalBudgetTrackingApp/index.ts
#	src/originalBudgetTrackingApp/outputVendors/index.js
# Conflicts:
#	src/components/app/Exporters.vue
#	src/components/app/exporters/ExpansionPanel.vue
#	src/components/app/exporters/Exporter.vue
#	src/components/app/exporters/JsonExporter.vue
# Conflicts:
#	src/components/app/exporters/Exporter.vue
…tracking into exportersBaruch

# Conflicts:
#	src/components/app/Exporters.vue
@brafdlog
Copy link
Owner Author

@baruchiro I went over the changes, looks fine on my end. If you approve my changes I will merge

Comment on lines 63 to 66
deleteAccountMapping(accountNumberToDelete) {
// @ts-ignore
this.$emit('deleteAccountMapping', accountNumberToDelete);
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have a bug here:

  1. Add new Account to Ynab item.
  2. Set value for Account number.
  3. Try to delete this item.

Also, the + button is stuck after that (and maybe without these steps)

searchlist2

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't research this deeply. Just a question (I'm not sure how Two Way Binding works, I'm always googling it), why you're using these events and not just use v-model?

As I remember, v-model is shortcut for value prop and input event, so you can get an array of pairs, and add/remove/change it inside YnabAccountMappingTable, and emit it in any change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Solved. I don't know if my solution is better in all the aspects, but I think we need to use v-model.

Then I had a lot of problems, I worked on this a day, so first I managed to get it to work, and now we have something to improve.

import { ref } from '@vue/composition-api';
import { cloneDeep } from 'lodash';

const objToArrayOfKeyValueArray = (obj: Record<string, string>) => Object.keys(obj).map((key) => ({ key, value: obj[key] }));
Copy link
Owner Author

Choose a reason for hiding this comment

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

This code seems way more generic and unclear than it should be.

This is a very specific use case with specific values. The generic names and types here just make it harder to understand what is going on.

(cherry picked from commit 69e2a1c)
@baruchiro baruchiro merged commit fa90c01 into unifyRepos Oct 7, 2020
@baruchiro baruchiro deleted the exportersBaruch branch October 7, 2020 10:40
@baruchiro baruchiro linked an issue Oct 7, 2020 that may be closed by this pull request
brafdlog pushed a commit that referenced this pull request Dec 22, 2020
…-plugin-vue-6.2.1

Bump eslint-plugin-vue from 6.2.0 to 6.2.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ui for configuring exporters
3 participants