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

Add preview page edit functionality #140

Merged
merged 5 commits into from
Jul 3, 2017
Merged

Conversation

ikisler
Copy link
Contributor

@ikisler ikisler commented Jun 28, 2017

Added a new action, preview_edit, and made some changes in the edit action so they'd work together.

Resolves Preview changes #59

I had attempted to use the Windows batch file to create the proper index, and etc., but it wasn't working for me. Wound up making a quick adjustment to pack.php and running build.php from the browser. I committed all the files changed -- hopefully that is correct?

Let me know if any changes are needed. :3

Make changes to edit action, add preview_edit action, adjust styles
@ikisler
Copy link
Contributor Author

ikisler commented Jun 28, 2017

Oh shoot. I just realized that I named the new action with an underbar, rather than a dash like the other multi-word actions... I will fix after work tomorrow. I obviously need to go to bed for now. xP

Copy link
Owner

@sbrl sbrl left a comment

Choose a reason for hiding this comment

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

This is rather neat and elegant, if I do say so 😸 I've found a few things I'm unsure on though, if you wouldn't mind taking a look 😺

* @apiPermission Anonymous
*
* @apiUse PageParameter
* @apiParam {string} newpage Set to 'yes' if a new page is being created.
Copy link
Owner

Choose a reason for hiding this comment

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

You haven't included your preview-edit parameter here? This is useful because then when you rebuild the docs with make (or just with apidoc if you look at the command in the makefile), it parses these blocks to make an api documentation page.

$pagetext = $_POST['content'];

// Set the tags to the new tags, if needed
if(isset($_POST['tags']))
Copy link
Owner

Choose a reason for hiding this comment

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

Well done! I hadn't thought of that. It does need doing, doesn't it?

@@ -73,6 +73,8 @@ textarea { min-height: 10em; line-height: 1.3em; font-size: 1.25rem; }
textarea, textarea[name=content] + pre, textarea ~ input[type=submit], #search-box { width: calc(100% - 0.3rem); box-sizing: border-box; }
textarea ~ input[type=submit] { margin: 0.5rem 0; padding: 0.5rem; font-weight: bolder; }
.editform input[type=text] { width: calc(100% - 0.3rem); box-sizing: border-box; }
input.edit-page-button[name='submit-edit'] { width: calc(50% - 1.25rem); margin-right: 1rem }
Copy link
Owner

@sbrl sbrl Jun 28, 2017

Choose a reason for hiding this comment

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

What are these CSS additions for? Just curious.

Copy link
Owner

@sbrl sbrl Jun 28, 2017

Choose a reason for hiding this comment

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

If you're using a calc() for the width, then it's possible that a box-sizing might wind up sorting your issue more elegantly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are so that the Save and Preview buttons sit next to each other, rather than one after the other... Hopefully that description makes sense.

I'll check out box-sizing and see if that'd be a more appropriate solution.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks 😺


if(isset($_POST['preview-edit']) && isset($_POST['content'])) {
// preview changes
get_object_vars($actions)['edit']();
Copy link
Owner

Choose a reason for hiding this comment

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

This is neat! Very cool indeed. This suddenly makes the action system a whole lot more powerful. You should be able to use $actions->edit(); though, I would have thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try calling it with $actions->edit(); -- it throws a fatal error:
PHP Fatal error: Uncaught Error: Call to undefined method stdClass::edit()

It actually took me a bit to figure out how to call the function in this way -- I found you had done something similar elsewhere in the code, so I built off of that.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, cool! Yeah, we can do that then. I don't always remember what I did before :P

$page_tags = $_POST['tags'];

// Insert the "view" part of the page we're editing
$content .= parse_page_source($pagetext);
Copy link
Owner

Choose a reason for hiding this comment

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

A header would be nice, to tell people that it's just a preview, and they can continue editing below.


$content .= "<form method='post' name='edit-form' action='index.php?action=preview_edit&page=' class='editform'>
<input type='hidden' name='prev-content-hash' value='" . ((isset($old_pagetext)) ? sha1($old_pagetext) : sha1($pagetext)) . "' />
<textarea name='content' autofocus tabindex='1'>$pagetext</textarea>
Copy link
Owner

Choose a reason for hiding this comment

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

I've just noticed. This might be a security issue? We might need to wrap $pagetext in htmlentities(). I know I hadn't done this before, but it's just come to mind now :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is a security issue, since we're hashing it into a hexadecimal value before we put it on the page. If we do wrap it in htmlentities() here, then it'd also need to be done in the save action as well, to prevent it from showing the conflicts page erroneously.

Copy link
Owner

Choose a reason for hiding this comment

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

Hrm, I think you're right, since we're just echoing back the content the user sent us. So if there was some security issue, it would have to hit before the content is sent to us.

So yeah, on further consideration, I don't think this is an issue.

@ikisler
Copy link
Contributor Author

ikisler commented Jun 28, 2017

Thank you for the feedback! I will go through these in more detail later today and update where needed.

@sbrl
Copy link
Owner

sbrl commented Jun 28, 2017

Sounds good! Just comment again here once you're done 😺

And don't worry about the conflicts - I'll sort them out myself once you're done tweaking 😉

Adjust styles, add preview header, change underbar to dash.
@ikisler
Copy link
Contributor Author

ikisler commented Jun 29, 2017

Okay, I think I've addressed everything?

Please take a look and let me know if I need to make additional changes. =D

Copy link
Owner

@sbrl sbrl left a comment

Choose a reason for hiding this comment

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

I like it! I'm inclined to make a few minor tweaks before merging, though. Right now though I'm going to clone it down and take a proper look for myself!

build/index.php Outdated
@@ -260,7 +260,7 @@

.printable { padding: 2rem; }

h1 { text-align: center; }
h1, h4 { text-align: center; }
Copy link
Owner

Choose a reason for hiding this comment

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

h4 What is this used for?

build/index.php Outdated
* @apiDescription Gets a preview of the current edit state of a given page
* @apiName PreviewPage
* @apiPermission Anonymous
*
* @apiUse PageParameter
* @apiParam {string} newpage Set to 'yes' if a new page is being created.
* @apiParam {string} preview-edit Set to a value to preview an edit of a page.
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks :D


$content .= "<form method='post' name='edit-form' action='index.php?action=preview_edit&page=' class='editform'>
<input type='hidden' name='prev-content-hash' value='" . ((isset($old_pagetext)) ? sha1($old_pagetext) : sha1($pagetext)) . "' />
<textarea name='content' autofocus tabindex='1'>$pagetext</textarea>
Copy link
Owner

Choose a reason for hiding this comment

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

Hrm, I think you're right, since we're just echoing back the content the user sent us. So if there was some security issue, it would have to hit before the content is sent to us.

So yeah, on further consideration, I don't think this is an issue.

@@ -162,11 +167,11 @@
$page_tags = $_POST['tags'];

// Insert the "view" part of the page we're editing
$content .= parse_page_source($pagetext);
$content .= "<h4>To continue editing or save, scroll down...</h4>" . parse_page_source($pagetext);
Copy link
Owner

Choose a reason for hiding this comment

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

Ah, It's for this message! Thanks for including it. I'd be tempted to use a <p class="preview-message"><strong>Message here</strong></p> rather than a <h4>Message here</h4>.

Copy link
Owner

Choose a reason for hiding this comment

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

In fact that's a better idea, thinking about it. That way a natural <h4> that's part of the page doesn't get the text-align: center; applied to it when we don't want it to be.

@@ -73,8 +73,7 @@ textarea { min-height: 10em; line-height: 1.3em; font-size: 1.25rem; }
textarea, textarea[name=content] + pre, textarea ~ input[type=submit], #search-box { width: calc(100% - 0.3rem); box-sizing: border-box; }
textarea ~ input[type=submit] { margin: 0.5rem 0; padding: 0.5rem; font-weight: bolder; }
.editform input[type=text] { width: calc(100% - 0.3rem); box-sizing: border-box; }
input.edit-page-button[name='submit-edit'] { width: calc(50% - 1.25rem); margin-right: 1rem }
input.edit-page-button[name='preview-edit'] { width: calc(50% - 1.25rem); margin-left: 1rem }
input.edit-page-button[type='submit'] { width: 49.5%; box-sizing: border-box; }
Copy link
Owner

Choose a reason for hiding this comment

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

I might fiddle with this myself just before merging. That reminds me. I need to clone your branch and take a proper look at it working! :P

@sbrl
Copy link
Owner

sbrl commented Jun 29, 2017

I've just taken a look. It works well - I like it! I've got a few more niggles though:

  • The save button should be on the right, not the left (English is left-to-right, and you'll want to preview it before saving, not the other way around)
  • The message should read something more like "This is only a preview, so your edits haven't been saved! Scroll down to continue editing.", as having an ellipsis there doesn't feel right.

Sorry for all the niggles! I'm rather picky as I've been working on this thing for a while :P

Change order of save and preview buttons, tweak HTML and styling for preview message.
@ikisler
Copy link
Contributor Author

ikisler commented Jul 2, 2017

Finally got time to look at this again! Rearranged the save and preview buttons, and changed the preview warning message content, and also moved it to it's own class rather than an H4.

Don't worry about being picky -- software development is all in the details.

@sbrl
Copy link
Owner

sbrl commented Jul 2, 2017

@ikisler Awesome~! Thanks for the additions. And yes - it certainly according to my (limited) experience so far. I'll just take a final look. 2 last issues:

  • You seem to have updated the css, but not the preview message (it's still got the old text from what I can see here in a <h4>)
  • I'm not sure where, but somewhere along the line I think one of your commits fiddled with the linux file permissions on the files in this repository. It's not an issue though - I can sort them out in a separate commit after merging.

@ikisler
Copy link
Contributor Author

ikisler commented Jul 2, 2017

Augh, how did I do that? >.< The preview message is actually updated now.

I probably broke the linux file permissions because I'm doing development on a Windows machine? I'm not sure how to prevent that from happening again, though -- will do some reading and see what can be done about it.

@sbrl
Copy link
Owner

sbrl commented Jul 3, 2017

Thanks for the fixes! Let's see about a final test, and then a merge 😸

@sbrl sbrl merged commit e9381da into sbrl:master Jul 3, 2017
@sbrl
Copy link
Owner

sbrl commented Jul 3, 2017

First manual merge complete! Thanks for the contribution, @ikisler. I'll add you to the credits!

@sbrl
Copy link
Owner

sbrl commented Jul 3, 2017

Turns out at least some of those permissions issues were actually my fault too - they were left over from when I was dual-booting & had my code repos on an NTFS partition :P

I've cleaned them up now anyway :D

@sbrl
Copy link
Owner

sbrl commented Jul 3, 2017

I've found a small bug - if you preview a page that isn't the main page, the page GET parameter is not set correctly such that it think you're editing the main page after clicking preview. I'm fixing it now.

sbrl added a commit that referenced this pull request Jul 3, 2017
@ikisler
Copy link
Contributor Author

ikisler commented Jul 4, 2017

Yay, thanks so much! =D

@sbrl
Copy link
Owner

sbrl commented Jul 5, 2017

@ikisler Np! It's been a fun process for me, too - especially because it was my first pull request from someone else against any of my projects 😺

Oh and v0.14 beta 1 is out! If there aren't any bugs, I'll probably do a full release on Friday.

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.

2 participants