kblincoe / QualOpt_SE701

2 stars 15 forks source link

Completes Issue #21 Add email Template #119

Closed victorlian closed 6 years ago

victorlian commented 6 years ago

This pull request should finish the feature request in #21 Add email templates. As this feature required a fair amount of code to write (front-end, back-end and tests), and hence a large amount of existing code to understand, I propose upgrading this feature from medium to large. @softeng-701

An email template can be applied, created, modified and deleted by a user. A user may only use templates created by him (not anyone else). These templates will be stored in the database.

There will also be a default template, and a "none" (empty) template. These templates are added by the front-end and can be used by any user.

Example usage

  1. Applying a template when editing email: 1
  2. Managing templates: 2
pulkitkalra commented 6 years ago

Tested and works as expected! Good work.

Just a couple of things that I noted while testing: (1) It allows you to save a template with the same name (probably shouldn't be allowed?). E.g. you can create many templates called 'default'.

(2) This is a small usability thing: It's a little tricky to work out which template is applied, since the name is in this drop-down here image I would think it'd be better to move it to the top just above the subject?

victorlian commented 6 years ago

@pulkitkalra Thanks for the feedback, does it mean the buttons would have to shift up as well? Or just the selector? As for the same name issue, it is legal under the current implementation. I'll look into disabling that. shouldn't be too difficult

pulkitkalra commented 6 years ago

@victorlian I reckon the buttons should go up as well, but I'll leave that up to you to decide :)

victorlian commented 6 years ago

I'm a little worries about the screen real-estate... don't want to make the modal too big. Which is why I left the controls in the footer. Could you post a screenshot on where you want the buttons be (a rough position maybe?) Try not to extend the modal too much :) Thanks!

pulkitkalra commented 6 years ago

I was thinking something like this (the back button can stay at the bottom). Having this at the top, it is clear which template you're modifying/ creating. What do you think?

image

victorlian commented 6 years ago

The one thing i don't like about this is the delete/update being far away from the save. (and so does the error message) Seems less coherent to me :(. Shall we leave this for mike to decide ? :P Should be an easy fix anyway

pulkitkalra commented 6 years ago

@victorlian oh whoops the save button, yeah that could go at the top as well? But this was just a suggestion, I'm happy to approve either way :)

victorlian commented 6 years ago

@pulkitkalra In that case I think I'll stick with the current design. It also needs to be consistent with the other "invitation email" tab, for which case the control is also at the bottom left corner. It doesn't make sense to shift that one to the top as well, because the "manage template" button should stay at the footer as it serves as a "forward" purpose, like the "next" button.

For consistency reasons, I think I'm going to leave all buttons/form control to the bottoms.

victorlian commented 6 years ago

Thanks guys. It has been approved and is now ready to be merged. @softeng-701

victorlian commented 6 years ago

The commit above is the squashed version. Code can now be merged. Please also notice the upgrade-to-large issue request in the post.

softeng-701 commented 6 years ago

@victorlian Please resolve the conflicts!

victorlian commented 6 years ago

@softeng-701 merge conflict resolved, was a single line conflict, resolved by taking both changes, now able to be merged.

(any updates on upgrading the size of the issue?)

softeng-701 commented 6 years ago

@victorlian still the conflict exists. Can you kindly fix, thanks?

I am not sure about which upgrade you are talking about, please explain, thanks.

victorlian commented 6 years ago

@softeng-701 More conflicts are now resolved. Regarding the upgrade-to-large request, Please scroll to the top and read the first post OR read #21.

softeng-701 commented 6 years ago

@victorlian Updated the size of #21 👍