Skip to content

KCI-156 #36

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 31, 2021
Merged

KCI-156 #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c36802
fix(icons): added required kingscode icons
jvhellemondt Mar 29, 2021
ec9c70a
revert(resource): reverted component to original state
jvhellemondt Mar 29, 2021
63446ff
refactor(readme): updated testing instructions
jvhellemondt Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,15 @@ as you like in this plugins. The most important choices are:
To test/develop the kingscode scaffold you can locally invoke this plugin.
create a test project (`vue create scaffold-tester`) just like above.

browse to the scaffold-tester folder, which was just created.

install vuetify

`vue add vuetify`

Comment on lines +57 to +62
Copy link
Contributor

@ricardovanlaarhoven ricardovanlaarhoven Mar 29, 2021

Choose a reason for hiding this comment

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

there is much more then just vuetify, So should this line be here?

just like above.
my referer to the installation is not really notable, perhaps just change that

Copy link
Contributor Author

@jvhellemondt jvhellemondt Mar 29, 2021

Choose a reason for hiding this comment

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

It is the full instruction to locally test the cli:
image

Copy link
Contributor

@ricardovanlaarhoven ricardovanlaarhoven Mar 30, 2021

Choose a reason for hiding this comment

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

No it isn't. Just like i said before there is much more then vuetify.

vue router
vuex
dart sass
..
..

For development you need to follow the instructions "just like above"
You could add those instructions just like you've added vuetify again at this place. But then you should remove the "just like above"
When you do that we'd have to maintain that at two places so i'm opting to revert your change and just make the line "just like above" more notable.

Something like this:

### Development
- To test/develop the kingscode scaffold you can locally invoke this plugin.
create a test project (`vue create scaffold-tester`) just like in the Installation part, you can follow those instructions and continue here.

- install the cli-plugin locally

`npm install path\to\local\vue-cli-plugin-kingscode-scaffold`

- invoke the plugin

`vue invoke kingscode-scaffold`

Copy link
Contributor Author

@jvhellemondt jvhellemondt Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why you keep pushing. The only fact is, my icon changes made Vuetify the only one mandatory for this cli (according to the instructions). Vue router, vuex and even dart sass (the dart sass thing is not required, if you don't select css pre-processors), are optional (according to the instructions. I did no changes in that). Thus without 'adding' vuetify, the development instruction is incomplete.

I do not mind changing the 'just like above' part. However, I think it's still valid.

Copy link
Contributor

@ricardovanlaarhoven ricardovanlaarhoven Mar 31, 2021

Choose a reason for hiding this comment

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

Let's call about this.
BTW PR was already approved so i'm not pushing but in my opinion this adds more confusion instead of removing it.

install the cli-plugin locally

`npm install path\to\local\vue-cli-plugin-kingscode-scaffold`
`npm i path\to\local\vue-cli-plugin-kingscode-scaffold`

invoke the plugin

Expand Down
8 changes: 2 additions & 6 deletions generator/templates/Crud/src/components/Resource.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,7 @@ export default {
this.beforeOpenCreate(selected);
}
if (this.modelType) {
const Model = this.modelType()
this.createForm.values = new Model;
this.createForm.values = new this.modelType();
}
},
beforeOpenUpdateHandler(selected) {
Expand All @@ -227,10 +226,7 @@ export default {
this.updateForm.values = selected[0];
return;
}

const Model = this.modelType()
const instance = new Model
this.updateForm.values = instance.mapResponse(selected[0]);
this.updateForm.values = new this.modelType().mapResponse(selected[0]);
},
},
};
Expand Down
4 changes: 4 additions & 0 deletions generator/templates/Default/src/plugins/vuetify/icons/kingscode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ const identifiers = {
fileExcel: ['fas', 'file-excel'],
filePowerpoint: ['fas', 'file-powerpoint'],
filePdf: ['fas', 'file-pdf'],

add: ['fas', 'plus'],
plus: ['fas', 'plus'],
minus: ['fas', 'minus'],
};

export default {
Expand Down
2 changes: 0 additions & 2 deletions generator/templates/Default/src/plugins/vuetify/icons/vuetify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,6 @@ const identifiers = {
last: ['fas', 'step-forward'],
unfold: ['fas', 'arrows-alt-v'],
file: ['fas', 'paperclip'],
plus: ['fas', 'plus'],
minus: ['fas', 'minus'],
};

export default { icons, identifiers, };