-
Notifications
You must be signed in to change notification settings - Fork 24
[#37] Update design of confirmation letter #117
base: master
Are you sure you want to change the base?
Conversation
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"/> | ||
</head> | ||
<body style="marin: 8px; padding: 0px; max-width: 768px"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marGin
<center><img style="width: 200px" width="200px" src="/img/gdg_ukraine_logo.png"></center> | ||
<p style="margin-left: 20px; margin-bottom: 20px; font-family: Lato, Arial, sans-serif">Hey, pal!</p> | ||
<p style="margin-left: 20px; font-family: Lato, Arial, sans-serif"> | ||
We are writing to you, because you have registered to "{{ event.title }}". We just want to clarify, whether you are going to attend the event, or you have another plans. You can get more info about an event right <a href="{{}}">here</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+80 chars lines is more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about comma before "whether"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<p style="margin-left: 20px; font-family: Lato, Arial, sans-serif"> | ||
We are writing to you, because you have registered to "{{ event.title }}". We just want to clarify, whether you are going to attend the event, or you have another plans. You can get more info about an event right <a href="{{}}">here</a>. | ||
</p> | ||
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If you are going to attend, press the button below.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I don't want to scroll horizontally
We are writing to you, because you have registered to "{{ event.title }}". We just want to clarify, whether you are going to attend the event, or you have another plans. You can get more info about an event right <a href="{{}}">here</a>. | ||
</p> | ||
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If you are going to attend, press the button below.</p> | ||
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If no, just ignore this email.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Otherwise
. Seems better for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both are valid
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If no, just ignore this email.</p> | ||
<a href="{{ confirm.url | e }}" style="text-decoration: none"> | ||
<div style="margin: 10px auto; background-color: blue; width: 160px; height: 35px; color: white; text-align: center; vertical-align: baseline; border-radius: 3px; border-color: blue" width="160px" height="35px" bgcolor="blue"> | ||
<div style="padding: 6px; font-family: Lato, Arial, sans-serif">I am going to attend</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attend -> come
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the button text shorter. Smth like I'll be there
or I will come
...
<center><img style="width: 200px" width="200px" src="/img/gdg_ukraine_logo.png"></center> | ||
<p style="margin-left: 20px; margin-bottom: 20px; font-family: Lato, Arial, sans-serif">Hey, pal!</p> | ||
<p style="margin-left: 20px; font-family: Lato, Arial, sans-serif"> | ||
We are writing to you, because you have registered to "{{ event.title }}". We just want to clarify, whether you are going to attend the event, or you have another plans. You can get more info about an event right <a href="{{}}">here</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about comma before "whether"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text may still need some fixes
</head> | ||
<body style="marin: 8px; padding: 0px; max-width: 768px"> | ||
<center><img style="width: 200px" width="200px" src="/img/gdg_ukraine_logo.png"></center> | ||
<p style="margin-left: 20px; margin-bottom: 20px; font-family: Lato, Arial, sans-serif">Hey, pal!</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pal -> rand('dude', 'man', 'pal', etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking about using participant's name here:
Hi {{ u.name or 'friend' }}
As for pal
, dude
, man
and so on, I believe we should replace it with something less gender-specific. You know, there can be female participants as well... =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anxolerd pal
is not gender-specific, as I think.... But that's good idea about {{ u.name or 'placeholder' }}
We are writing to you, because you have registered to "{{ event.title }}". We just want to clarify, whether you are going to attend the event, or you have another plans. You can get more info about an event right <a href="{{}}">here</a>. | ||
</p> | ||
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If you are going to attend, press the button below.</p> | ||
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If no, just ignore this email.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise
?
</head> | ||
<body style="marin: 8px; padding: 0px; max-width: 768px"> | ||
<center><img style="width: 200px" width="200px" src="/img/gdg_ukraine_logo.png"></center> | ||
<p style="margin-left: 20px; margin-bottom: 20px; font-family: Lato, Arial, sans-serif">Hey, pal!</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking about using participant's name here:
Hi {{ u.name or 'friend' }}
As for pal
, dude
, man
and so on, I believe we should replace it with something less gender-specific. You know, there can be female participants as well... =)
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If no, just ignore this email.</p> | ||
<a href="{{ confirm.url | e }}" style="text-decoration: none"> | ||
<div style="margin: 10px auto; background-color: blue; width: 160px; height: 35px; color: white; text-align: center; vertical-align: baseline; border-radius: 3px; border-color: blue" width="160px" height="35px" bgcolor="blue"> | ||
<div style="padding: 6px; font-family: Lato, Arial, sans-serif">I am going to attend</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the button text shorter. Smth like I'll be there
or I will come
...
</a> | ||
<p style="margin-left: 20px; font-family: Lato, Arial, sans-serif"> | ||
Sincerely yours,<br> | ||
friends from GDG Ukraine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a group related to GDG event. Probably we should be more specific and use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truth. Will fix it
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If you are going to attend, press the button below.</p> | ||
<p style="margin: 0px 20px; font-family: Lato, Arial, sans-serif">If no, just ignore this email.</p> | ||
<a href="{{ confirm.url | e }}" style="text-decoration: none"> | ||
<div style="margin: 10px auto; background-color: blue; width: 160px; height: 35px; color: white; text-align: center; vertical-align: baseline; border-radius: 3px; border-color: blue" width="160px" height="35px" bgcolor="blue"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for checking. I think I'll fix it with smaller text in button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not to use rem on text nodes, and em on thinks, that attached to nodes? This will bring more responsibility to letter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work for the most of email clients?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz, @ValkoVolodya I was not sure it does, so I used px
. Most guides used it, so I did so 😄
<meta name="viewport" content="width=device-width, initial-scale=1.0"/> | ||
</head> | ||
<body style="marin: 8px; padding: 0px; max-width: 768px"> | ||
<center><img style="width: 200px" width="200px" src="/img/gdg_ukraine_logo.png"></center> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src="/img/..."
This is bad. Make the url absolute (starting from https)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, you can use group-specific logo here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is <base href="https://gdg.org.ua"/>
in the headings of letter, so the absolute url is not required.
And what about group-specific logo - will think about it
<body style="marin: 8px; padding: 0px; max-width: 768px"> | ||
<center><img style="width: 200px" width="200px" src="/img/gdg_ukraine_logo.png"></center> | ||
<p style="margin-left: 20px; margin-bottom: 20px; font-family: Lato, Arial, sans-serif">Hey, pal!</p> | ||
<p style="margin-left: 20px; font-family: Lato, Arial, sans-serif"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really impossible to write paragraph styles only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only in case of using some tool, which would inline everything from CSS declarations into html tags. It's a must for web UI based email clients to have styles inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz: oh crap :(
<center><img style="width: 200px" width="200px" src="/img/gdg_ukraine_logo.png"></center> | ||
<p style="margin-left: 20px; margin-bottom: 20px; font-family: Lato, Arial, sans-serif">Hey, pal!</p> | ||
<p style="margin-left: 20px; font-family: Lato, Arial, sans-serif"> | ||
We are writing to you, because you have registered to "{{ event.title }}". We just want to clarify, whether you are going to attend the event, or you have another plans. You can get more info about an event right <a href="{{}}">here</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for this paragraph, user sees no value to himself (and no reason in consequence) to click the button. What about smth like:
We are glad you are interested in {{ event.title }}.
There is one small step left to complete your registration...
?
We have to give some value to user so he/she could see the reason to click that button, not just ignore this email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how it spells. It's a bit... business-formal or somehow like that.
Also that staff about one little step, tells that user has to do more stuff for us (while noone likes to do a lot of staff for someone - event just click on a button). And, also, it generates more text he has to read. Do you read all those long emails, the only purpose of which is to confirm that you are actually going to go somewhere?
I think, it's better to make it like that:
begin-part
Thanks for registering to the event "{{ event.title }}".
We are really looking forward to see you there, but we must clarify that you will come. If you will, press "I will come".
If not, no action is needed, just ignore this letter.
end-part
This is kinda draft and has some issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz thanks :-) Didn't knew that XD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knew -> know
Hey, @bluebirrrrd ! We know you're a super-dupa busy person, but could you please help us with the text of confirmation letter? We really need your help and advise ;) |
Hey guys, I'll skip the part with criticism and just modify the @cleac's version below.
Thanks for registering to the event "{{ event.title }}".
If you have decided not to come, just ignore this letter.
Also, adding some time limits for confirmation would be nice. I skipped this part since we don't have any features for that at the moment. [Advice] Please read The art of business correspondence by Sasha Karepina before writing any letters of confirmation. I believe all of you will find it very helpful for further mailing (& discussions on github as well). |
Currently have some work and univ's routine. Will do ASAP 😄 |
74621e8
to
bed6b74
Compare
@cleac status? |
@webknjaz WIP cause too much time left from time I started this PR |
@cleac ping |
Hi all, sorry for losing touch. I will finish this PR today, currently working on it 😃 |
I decided to use mjml framework for this. After trying several of them, it looks the best and easiest as for me. I will make detached header with GDG logo and text which have to be in every single email, like The only drawback I've found - we will have to put only absolute urls in the letter. |
sounds ok |
@cleac I've implemented a set of |
@webknjaz awesome! :-) |
@webknjaz is there any doc on |
It seems that thats all on task :-) |
@@ -0,0 +1,6 @@ | |||
<mj-section> | |||
<mj-column> | |||
<mj-image width="200" src="/img/gdg_ukraine_logo.png"></mj-image> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to have templated src
value via url_for_static
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tried it, will look through
@@ -19,6 +19,7 @@ TOX=$(PENV_BIN_PATH)tox | |||
USER_SHELL=/bin/zsh | |||
ACTIVATE_ENV=if test -d ./$(PENV); then . ./$(PENV)/bin/activate; fi; if test -f ./.exports; then . ./.exports; fi | |||
USE_NVM=if test -d ~/.nvm; then . ~/.nvm/nvm.sh; nvm use 5.0.0 ; fi | |||
EMAIL_DIRECTORY=src/GDGUkraine/templates/email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make make
learn about dotenv ^^
Makefile
Outdated
@@ -144,3 +145,6 @@ stop-prod: | |||
.PHONY: environ-regen | |||
environ-regen: | |||
bin/mk-environ-file.sh | |||
|
|||
mjml: | |||
@bin/build_mjml.sh ${EMAIL_DIRECTORY} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you shouldn't suppress output here
Makefile
Outdated
@@ -144,3 +145,6 @@ stop-prod: | |||
.PHONY: environ-regen | |||
environ-regen: | |||
bin/mk-environ-file.sh | |||
|
|||
mjml: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may also want to add front-deps
target as a dependency and extend it to install utility you need :)
I'll write down some tests later today |
Makefile
Outdated
@@ -142,9 +143,10 @@ stop-prod: | |||
|
|||
.env: environ-regen | |||
|
|||
.PHONY: environ-regen | |||
.PHONY: environ-regen mjml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you want to regen mjml along with .env
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz : emmm... Is someone going to call make .PHONY
? 'R 'u mad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, and this is an incorrect syntax for .phony
bin/build_mjml.sh
Outdated
@@ -18,11 +18,11 @@ END | |||
MJML_REGEX="^[^_].+\.mjml$" | |||
|
|||
if [ "$1" = 'help' -o "$1" = 'h' -o "$1" = '--help' ]; then | |||
echo "$USAGE" | |||
echo -n "$USAGE\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nonsense. Just don't use -n
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTF?
Do I understand that correctly that you disable default trailing newline and print yours instead? Why would you ever need that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I just did :%s/printf/echo -n/g
😅 😅 😅 😅
@@ -13,12 +13,14 @@ OPEN_URL=xdg-open | |||
MIGRATOR=$(PENV_BIN_PATH)alembic -c config/alembic.ini | |||
BWR=bower | |||
NPM=npm | |||
MJML=mjml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way I literally have no idea why have both you and @webknjaz created these aliases.
Can anyone explain that to me, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anxolerd the same reason, why there are predefined
bin/build_mjml.sh
Outdated
@@ -18,11 +18,11 @@ END | |||
MJML_REGEX="^[^_].+\.mjml$" | |||
|
|||
if [ "$1" = 'help' -o "$1" = 'h' -o "$1" = '--help' ]; then | |||
echo -n "$USAGE\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz Why do I need this USAGE
? Just for little help if someone will want to use this script in other place without reading the code. I wasn't going to add this script to the Makefile
while writing it, so I decided to add some help and now I have no idea why I should delete it 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, why did you remove -n
? (again)
bin/build_mjml.sh
Outdated
else | ||
if [ -z $dirs ] | ||
then | ||
dirs=$arg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if arg
contains spaces?
bin/build_mjml.sh
Outdated
if [ "$arg" = '-m' -o "$arg" = '--minify' ]; then | ||
minify='-m' | ||
else | ||
if [ -z $dirs ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if dirs
contains spaces this fails with error
bin/build_mjml.sh
Outdated
fi | ||
done | ||
|
||
if [ -z $dirs ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if dirs
contains spaces this fails with error
bin/build_mjml.sh
Outdated
done | ||
|
||
if [ -z $dirs ]; then | ||
echo -n 'Please, specify directory, where the files to build are!\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
echo 'Please, specify ...to build are!'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, what abound sending this to stderr
?
bin/build_mjml.sh
Outdated
fi | ||
|
||
for dirname in $dirs; do | ||
if [ -d $dirname ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails if dirname
contains spaces
bin/build_mjml.sh
Outdated
for dirname in $dirs; do | ||
if [ -d $dirname ]; then | ||
echo -n "Building for directory $dirname " | ||
for file in $(ls $dirname | grep -E $MJML_REGEX ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably $dirname
and $MJML_REGEX
should be quoted
bin/build_mjml.sh
Outdated
if [ $? -eq 0 ]; then | ||
echo -n '.' | ||
else | ||
echo -n 'ERROR\nAn error occured, please check it out\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about sending this to stderr
?
bin/build_mjml.sh
Outdated
exit 3 | ||
fi | ||
done | ||
echo -n ' Done\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
echo ' Done'
bin/build_mjml.sh
Outdated
done | ||
echo -n ' Done\n' | ||
else | ||
echo -n "\nDirectory '$dirname' was not found: skipping\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
echo "\nDirectory ... skipping"
How to build mjml emails: 1. Install mjml globally from npm repository 2. Run make mjml everytime you edit email All the emails templates now have to be in directory src/GDGUkraine/templates/email. If not, add directory to EMAIL_DIRECTORY in Makefile - Add mjml tempaltes building - Add _gdg_logo.mjml file with gdg logo in it
- Add mjml_build script to .PHONY and frontend-deps targets - Make gdg_logo take absolute url - Make build_mjml.sh use `echo -n` instead of `printf` - Remove build html file from diff
- Fix usage of .PHONY target - Fix `echo -n` issue
- Make errors be thrown to stderr
Resolves #37
Implement the updated design of confirmation letter.
@webknjaz @bluebirrrrd @anxolerd: give your suggestions of text improvements