[feature/qrpreview] Change "Full Editor" button to "Full Editor & Preview" on Quick Reply #659

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
7 participants
@imkingdavid
Contributor

imkingdavid commented Mar 25, 2012

As per IRC discussion, button language changed to include "& Preview".

I have tested this and it works.

http://tracker.phpbb.com/browse/PHPBB3-10726

@@ -38,13 +38,13 @@
{S_FORM_TOKEN}
{QR_HIDDEN_FIELDS}
<input type="submit" accesskey="s" tabindex="6" name="post" value="{L_SUBMIT}" class="button1" />&nbsp;
- <input type="submit" accesskey="f" tabindex="7" name="full_editor" value="{L_FULL_EDITOR}" class="button2" />&nbsp;
+ <input type="submit" accesskey="p" tabindex="7" name="preview" value="{L_PREVIEW}" class="button2" />&nbsp;

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Mar 25, 2012

Contributor

If we change this so its like the posting page, I would be in favor of ordering the buttons just like they are on the posting page.

@nickvergessen

nickvergessen Mar 25, 2012

Contributor

If we change this so its like the posting page, I would be in favor of ordering the buttons just like they are on the posting page.

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 25, 2012

Contributor

So put preview before submit? I can do that later today.

@imkingdavid

imkingdavid Mar 25, 2012

Contributor

So put preview before submit? I can do that later today.

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 25, 2012

Contributor

And done.

@imkingdavid

imkingdavid Mar 25, 2012

Contributor

And done.

imkingdavid added some commits Mar 25, 2012

[feature/qrpreview] Move Preview button before Submit button
As requested, the QR buttons are arranged similar to the Posting Editor

PHPBB3-10726
@bantu

This comment has been minimized.

Show comment Hide comment
@bantu

bantu Mar 27, 2012

Member

This is kind of a controversial patch. I'm not convinced that removing L_FULL_EDITOR is a good idea because the whole point of having L_FULL_EDITOR instead of L_PREVIEW or L_SUBMIT was so people see how to get to the full editor. So, more feedback please.

Member

bantu commented Mar 27, 2012

This is kind of a controversial patch. I'm not convinced that removing L_FULL_EDITOR is a good idea because the whole point of having L_FULL_EDITOR instead of L_PREVIEW or L_SUBMIT was so people see how to get to the full editor. So, more feedback please.

@imkingdavid

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 27, 2012

Contributor

I think we're assuming our users are dumber than they are. MyBB has "preview post" on their quick reply, and I'm pretty sure that everyone there understands that when you preview, it goes to the main posting page. Anyway, it would only take one try to figure it out if anyone did have a question. @bantu You're the first opposition I've had to this patch so far, so I don't know if the issue is really that big.

A compromise: instead of changing the text of the button, we could change only its functionality? AKA make the button act like a preview button while retaining the text "full editor"

Contributor

imkingdavid commented Mar 27, 2012

I think we're assuming our users are dumber than they are. MyBB has "preview post" on their quick reply, and I'm pretty sure that everyone there understands that when you preview, it goes to the main posting page. Anyway, it would only take one try to figure it out if anyone did have a question. @bantu You're the first opposition I've had to this patch so far, so I don't know if the issue is really that big.

A compromise: instead of changing the text of the button, we could change only its functionality? AKA make the button act like a preview button while retaining the text "full editor"

@p

This comment has been minimized.

Show comment Hide comment
@p

p Mar 27, 2012

Contributor

I had the same thoughts as bantu actually. There are two reasons to hit that button and this patch changes the label from one reason to the other reason. Something like "Preview/Full Editor" would cover both, although I don't know how ugly it would be.

What do you mean by "change only its functionality"? I thought functionality was the same as before?

Contributor

p commented Mar 27, 2012

I had the same thoughts as bantu actually. There are two reasons to hit that button and this patch changes the label from one reason to the other reason. Something like "Preview/Full Editor" would cover both, although I don't know how ugly it would be.

What do you mean by "change only its functionality"? I thought functionality was the same as before?

@imkingdavid

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 27, 2012

Contributor

@p no, currently the full editor button brings the user to the posting screen while retaining whatever was in the posting box. By changing the functionality (i.e. making it named "preview") it does that as well as display the "Preview" box above the posting screen, as if from the posting screen the user had hit Preview.

What I meant by changing only the functionality was keeping the button text "Full Editor" but making it preview what has been typed in.

EDIT: Saying "Preview/Full Editor" is unneeded. Ultimately, by previewing you are seeing the full editor as well. I don't see what the problem is here?

EDIT2: All this patch does is add onto the current QR behavior in order to mimic the current posting screen behavior to make the forms more consistent. I don't see a problem with previewing what you have in the box even if all you want to do is to get to the full editor from the QR.

Contributor

imkingdavid commented Mar 27, 2012

@p no, currently the full editor button brings the user to the posting screen while retaining whatever was in the posting box. By changing the functionality (i.e. making it named "preview") it does that as well as display the "Preview" box above the posting screen, as if from the posting screen the user had hit Preview.

What I meant by changing only the functionality was keeping the button text "Full Editor" but making it preview what has been typed in.

EDIT: Saying "Preview/Full Editor" is unneeded. Ultimately, by previewing you are seeing the full editor as well. I don't see what the problem is here?

EDIT2: All this patch does is add onto the current QR behavior in order to mimic the current posting screen behavior to make the forms more consistent. I don't see a problem with previewing what you have in the box even if all you want to do is to get to the full editor from the QR.

@senky

This comment has been minimized.

Show comment Hide comment
@senky

senky Mar 27, 2012

Contributor

@imkingdavid maybe call button like "Preview and go to Full Editor" (or some shortcut as it is really long for button)? Common users don't need to know Preview also redirects to full editor...

Contributor

senky commented Mar 27, 2012

@imkingdavid maybe call button like "Preview and go to Full Editor" (or some shortcut as it is really long for button)? Common users don't need to know Preview also redirects to full editor...

</fieldset>
<span class="corners-bottom"><span></span></span></div>
</div>
</form>
</noscript>
-<form method="post" action="{U_QR_ACTION}">
+<form method="post" action="{U_QR_ACTION}" id="postform">

This comment has been minimized.

Show comment Hide comment
@naderman

naderman Mar 27, 2012

Owner

This is not a good idea. The id refers to the full editor. While it might be a good idea to add an id here, it should not be the same as for the full editor.

@naderman

naderman Mar 27, 2012

Owner

This is not a good idea. The id refers to the full editor. While it might be a good idea to add an id here, it should not be the same as for the full editor.

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 27, 2012

Contributor

An ID is needed for the onclick event that appends #preview to the action of the form submission. I'll change the ID to something like qrpostform instead.

@imkingdavid

imkingdavid Mar 27, 2012

Contributor

An ID is needed for the onclick event that appends #preview to the action of the form submission. I'll change the ID to something like qrpostform instead.

@imkingdavid

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 27, 2012

Contributor

Unfiltered IRC Discussion: https://gist.github.com/2221263

Contributor

imkingdavid commented Mar 27, 2012

Unfiltered IRC Discussion: https://gist.github.com/2221263

phpBB/language/en/viewtopic.php
@@ -61,7 +61,7 @@
'FILE_NOT_FOUND_404' => 'The file <strong>%s</strong> does not exist.',
'FORK_TOPIC' => 'Copy topic',
- 'FULL_EDITOR' => 'Full Editor',
+ 'FULL_EDITOR' => 'Full Editor & Preview',

This comment has been minimized.

Show comment Hide comment
@naderman

naderman Mar 27, 2012

Owner

should be & amp; since it's output as HTML directly.

@naderman

naderman Mar 27, 2012

Owner

should be & amp; since it's output as HTML directly.

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 27, 2012

Contributor

Alright, i'll change that

@imkingdavid

imkingdavid Mar 27, 2012

Contributor

Alright, i'll change that

@@ -60,8 +60,8 @@
<fieldset class="submit-buttons">
{S_FORM_TOKEN}
{QR_HIDDEN_FIELDS}
- <input type="submit" accesskey="s" tabindex="6" name="post" value="{L_SUBMIT}" class="button1" />&nbsp;
- <input type="submit" accesskey="f" tabindex="7" name="full_editor" value="{L_FULL_EDITOR}" class="button2" />&nbsp;
+ <input type="submit" accesskey="f" tabindex="6" name="preview" value="{L_FULL_EDITOR}" class="button2" onclick="document.getElementById('postform').action += '#preview';" />&nbsp;

This comment has been minimized.

Show comment Hide comment
@naderman

naderman Mar 27, 2012

Owner

Actually looking at this again. Is there a reason not to just always include #preview in the action? If it ends up going to a page without a preview it would just be ignored, no? So no need for inline javascript here.

@naderman

naderman Mar 27, 2012

Owner

Actually looking at this again. Is there a reason not to just always include #preview in the action? If it ends up going to a page without a preview it would just be ignored, no? So no need for inline javascript here.

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 28, 2012

Contributor

Someone already asked this on IRC. I'm just doing what is already done in the posting page. Otherwise, we get questions when people notice that every time they submit a post from quick reply there is "#preview" at the end of the URL. Sure it doesn't hurt anything, but it's inconsistent behavior to what is already there.

@imkingdavid

imkingdavid Mar 28, 2012

Contributor

Someone already asked this on IRC. I'm just doing what is already done in the posting page. Otherwise, we get questions when people notice that every time they submit a post from quick reply there is "#preview" at the end of the URL. Sure it doesn't hurt anything, but it's inconsistent behavior to what is already there.

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Mar 28, 2012

Contributor

I asked the same, and im still in favor of doing it (add it to action)

@nickvergessen

nickvergessen Mar 28, 2012

Contributor

I asked the same, and im still in favor of doing it (add it to action)

This comment has been minimized.

Show comment Hide comment
@naderman

naderman Mar 28, 2012

Owner

Well he's got a point. But I still don't like adding inline javascript for this. But there really isn't any other way, is there?

@naderman

naderman Mar 28, 2012

Owner

Well he's got a point. But I still don't like adding inline javascript for this. But there really isn't any other way, is there?

This comment has been minimized.

Show comment Hide comment
@p

p Mar 28, 2012

Contributor

includejs etc?

@p

p Mar 28, 2012

Contributor

includejs etc?

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 28, 2012

Contributor

Once jQuery is merged (is it? I haven't checked) we can add an ID to the submit button and then do $("#qrpost").click(...) to handle it instead of doing it inline. Or as @p suggested we could use normal javascript and include it.

@imkingdavid

imkingdavid Mar 28, 2012

Contributor

Once jQuery is merged (is it? I haven't checked) we can add an ID to the submit button and then do $("#qrpost").click(...) to handle it instead of doing it inline. Or as @p suggested we could use normal javascript and include it.

This comment has been minimized.

Show comment Hide comment
@naderman

naderman Mar 28, 2012

Owner

Well my concern with it wasn't so much the "inline" part but the "javascript" part. But I guess there's no way around it. jQuery has been merged a while ago, and indeed I'd prefer if this was in a script file.

@naderman

naderman Mar 28, 2012

Owner

Well my concern with it wasn't so much the "inline" part but the "javascript" part. But I guess there's no way around it. jQuery has been merged a while ago, and indeed I'd prefer if this was in a script file.

This comment has been minimized.

Show comment Hide comment
@senky

senky Mar 28, 2012

Contributor

So this should probably be changed to sth like that:
<input type="submit" id="preview_btn" accesskey="f" tabindex="6" name="preview" value="{L_FULL_EDITOR}" class="button2" onclick="document.getElementById('postform').action += '#preview';" />&nbsp;
and include some .js file with this content:

   $('#postform').attr('action', function(i, val) {
      return val + '#preview';
   });
});

... I think...

@senky

senky Mar 28, 2012

Contributor

So this should probably be changed to sth like that:
<input type="submit" id="preview_btn" accesskey="f" tabindex="6" name="preview" value="{L_FULL_EDITOR}" class="button2" onclick="document.getElementById('postform').action += '#preview';" />&nbsp;
and include some .js file with this content:

   $('#postform').attr('action', function(i, val) {
      return val + '#preview';
   });
});

... I think...

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 28, 2012

Contributor

I'm rusty on javascript and jquery, so thanks @senky. As for putting that in a file, is there already a file I shuold just add that into, or should I start a new file? if so, what are the naming guidelines? And do I then use , and if so does it go in this file or in overall_header? as I said, I'm not a big javascript developer at the moment...

EDIT: and you have an extra comma in that attr() call, right before function(i, val)...

@imkingdavid

imkingdavid Mar 28, 2012

Contributor

I'm rusty on javascript and jquery, so thanks @senky. As for putting that in a file, is there already a file I shuold just add that into, or should I start a new file? if so, what are the naming guidelines? And do I then use , and if so does it go in this file or in overall_header? as I said, I'm not a big javascript developer at the moment...

EDIT: and you have an extra comma in that attr() call, right before function(i, val)...

This comment has been minimized.

Show comment Hide comment
@senky

senky Mar 28, 2012

Contributor

@imkingdavid this code was written on the fly, so I am not sure it is according to coding guidelines or if it works - needs to be checked.

About question whether it goes to overall_header: I am not sure, but if no one have spoken about it at phpbb yet - JS files should be included just above closing body tag (</body>) to speed up loading of a page. Another possiblity is to use asynchronous JS loading... This probably needs to be discussed more at area51 or somewhere in case it is not discussed already...

and thanks for EDIT, I have fixed that.

@senky

senky Mar 28, 2012

Contributor

@imkingdavid this code was written on the fly, so I am not sure it is according to coding guidelines or if it works - needs to be checked.

About question whether it goes to overall_header: I am not sure, but if no one have spoken about it at phpbb yet - JS files should be included just above closing body tag (</body>) to speed up loading of a page. Another possiblity is to use asynchronous JS loading... This probably needs to be discussed more at area51 or somewhere in case it is not discussed already...

and thanks for EDIT, I have fixed that.

@VSEphpbb

This comment has been minimized.

Show comment Hide comment
@VSEphpbb

VSEphpbb Mar 28, 2012

Member

@imkingdavid Yes, jQuery has been merged in to 3.1. So any new javascript coding should be unobtrusive.

Member

VSEphpbb commented Mar 28, 2012

@imkingdavid Yes, jQuery has been merged in to 3.1. So any new javascript coding should be unobtrusive.

@imkingdavid

This comment has been minimized.

Show comment Hide comment
@imkingdavid

imkingdavid Mar 31, 2012

Contributor

Because I am still no good at merging in changes and managing conflicts and then building on it from there, I am going to close this PR and restart this based on the most recent develop (which has the other QR patch merged).

Contributor

imkingdavid commented Mar 31, 2012

Because I am still no good at merging in changes and managing conflicts and then building on it from there, I am going to close this PR and restart this based on the most recent develop (which has the other QR patch merged).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment