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

Output vendor interface #59

Merged
merged 34 commits into from
Jul 20, 2020
Merged

Output vendor interface #59

merged 34 commits into from
Jul 20, 2020

Conversation

baruchiro
Copy link
Collaborator

I started to define an interface for the output vendors.

I created a Json exporter, and now it is shown in the UI, but it still doesn't work- the next step is to remove the MainTable and replace it with a big run button.

So I think we can merge this, and continue to work on

  • Implement the interface for the rest of the output vendors (it will require more decisions about the interface)
  • See all the TODOs I wrote, maybe before merging this PR.
  • Create a big run button.
  • Implement the full process- from importing to exporting.

I will start with the TODOs and then with the run.
Maybe you @brafdlog can convert the output vendors to the new interface, because you wrote them.

@baruchiro baruchiro requested a review from brafdlog July 14, 2020 19:04
src/App.vue Outdated
@@ -3,6 +3,7 @@
</template>

<script>
// TODO why we need this component?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We creating the root component in main.js. The root component used the App.vue component, and this component do nothing except the same thing- use the next component.

So I'm removing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually we doing so many changes, no need to describe each one.

Comment on lines 11 to 13
// TODO: this plugin just copy the config state,
// if the backup will fail, we will have two configs- filesystem and store
// Maybe we need to backup in the action
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #51 (comment)
I'm keeping this comment until we discuss and decide if we want to change this.

Comment on lines 11 to 12
// TODO do we want to inject all props from 'outputVendors' (by passing context.data),
// or we want to sanitize and select specific properties?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The context.data is the object from v-bind. In my code, I injected the whole fields data from outputVendor.

The question is if we want to sanitize or filter some properties from the fields before it affects the UI.
Because now, I'm giving to outputVendor developer full control on the UI, because what he will define in the fields will become to component properties.

Copy link
Owner

Choose a reason for hiding this comment

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

As I wrote in another comment, I think we should less dynamic in the outputVendors. There won't be that many. I think they should have a clear interface for their common functionality and extentions for vendor specific behaviour and the ui will match this - common component for the common code and specific components for vendor specific behaviour.
That is what I think at the moment, I will spend some time on the output vendor api and then will be smarter

src/main.js Outdated
import store from './store';

initializeReporter();

process.on('unhandledRejection', (error) => {
debugger;
Copy link
Owner

Choose a reason for hiding this comment

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

Probably don't want a debugger here :)

src/main.js Outdated Show resolved Hide resolved
Co-authored-by: Baruch Odem (Rothkoff) <baruchiro@gmail.com>
Vue.use(Vuex);

const store = new Vuex.Store({
modules,
plugins: [configPersistPlugin('config')],
// TODO: https://github.com/brafdlog/budget-tracking/issues/51#issuecomment-658835043
Copy link
Owner

Choose a reason for hiding this comment

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

See comment in this thread: #51 (comment)

@@ -0,0 +1,10 @@
import path from 'path';

const files = require.context('.', true, /\.\/.+\/index\.js$/);
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer a less dynamic approach. Especially in apis. We are not going to be adding new vendors every day so the value of dynamic code is low IMO. I will spend some time on the output vendors code

baruchiro and others added 3 commits July 17, 2020 12:05
We will rethink about the status, maybe even after the merge.
It is better to develop it again, instead of trying to keep it.

Co-authored-by: Jonathan <brafdlog@gmail.com>
@brafdlog brafdlog mentioned this pull request Jul 17, 2020
@brafdlog
Copy link
Owner

@baruchiro As we discussed this pr is now in a state where the application is working and ts is set correctly. I suggest we merge it and start a new branch for the output vendors and types. WDYT?

@baruchiro
Copy link
Collaborator Author

Right, let's merge it

@brafdlog brafdlog merged commit 2e0e368 into unifyRepos Jul 20, 2020
@brafdlog brafdlog deleted the outputVendorInterface branch July 20, 2020 15:14
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.

2 participants