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

added edit templates #22

Closed
wants to merge 4 commits into from
Closed

added edit templates #22

wants to merge 4 commits into from

Conversation

tonykoval
Copy link
Contributor

No description provided.

@lmenezes
Copy link
Owner

lmenezes commented Nov 8, 2016

@tonyko hey, thanks for the PR. I have a few comments on it though:

  1. Instead of editing the app.js file directly, you should edit the source file(https://github.com/lmenezes/cerebro/blob/master/src/app/components/templates/controller.js)
  2. The mechanics for updating an index template could be managed differently: the pencil icon only loads the data into the form, and what actually makes it an update instead of a create, is the name of the template already existing in the templates list. Right now, if I edit a template but change the name of it, it would still seem like an update when it isn't. What do you think?
  3. Would be nice having a test here for this: https://github.com/lmenezes/cerebro/blob/master/tests/components/templates/controller.tests.js
  4. For generating the app.js, just grunt grunt build, and make sure grunt test also passes :)

@tonykoval
Copy link
Contributor Author

Updated. Thanks for comments.

@lmenezes
Copy link
Owner

@tonyko hey! thanks for updating this. But I think we had a bit of misunderstanding on the workflow for this :)

What I meant was:

1- User clicks on pencil icon
2- Information for the template is loaded on the same form as the one used for creating
3- labels for the form/button(create new template/create -> edit template/save) should be rendered based on the template name of the form being present on the list of templates we have loaded.

This means that:
1- We dont really need the update method/route on the backend. Creating a template with the same name will just overwrite the current one.
5- We also don't really need any extra logic on the frontend, its just a regular create(even though here one could argue that a confirm dialog would be helpful).

Not sure if I managed to explain this well, but if not, just let me know :)

and thanks once again!

@tonykoval
Copy link
Contributor Author

Updated.

@tonykoval tonykoval mentioned this pull request Nov 26, 2016
@lmenezes
Copy link
Owner

@tonyko hey
sorry that it took me long to answer this, and also that I didn't manage to explain properly the way I wanted this to be done. As I found sometime today, I did a simple implementation and pushed this. Will soon release a version with the change.

Thanks for submitting this, and sorry for not getting it merged.

@lmenezes lmenezes closed this Dec 13, 2016
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.

None yet

2 participants