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

New admin preview #3383

Merged
merged 5 commits into from Apr 12, 2017

Conversation

@BertrandBordage
Member

BertrandBordage commented Feb 17, 2017

Currently, the admin preview is easily breakable. You get an infinite spinner on the preview page when you:

  • refresh the preview page;
  • open a preview, submit or refresh the edit page then click again on preview.

As agreed with @tomdyson, I worked on a solution to fix these annoyances.

The solution was to avoid sending form data directly to the preview page. So data had to be stored somewhere, and I chose to store it in django session data. I automatically clean the session to remove old preview data to avoid increasing its weight too much. This is the best compromise I found to avoid creating an extra database table just for previews…

It also adds WAGTAIL_AUTO_UPDATE_PREVIEW, a setting that allows users to refresh a preview page to see new changes without having to click again on the preview button. I asked a few Wagtail users to know which behavior they preferred and it was fifty-fifty, so I decided to create this setting. Since this sends a new AJAX request on the edit page on each unsaved modification (which is a bit heavy), this is disabled by default. Note that there can be at max one AJAX request per second, to avoid performance issues, and requests are disabled while typing.

I noticed a browser performance issue while typing data, and thought at first it was coming from my changes, but it appears to be coming from something else since it also occurs on the master branch. I didn’t dig much, but it’s not happening on my deployed Wagtail 1.8 sites.

Another existing issue is that request.FILES is correctly sent to the preview page, but since files are not saved before rendering the page, it only leads to dead links. I chose to ignore request.FILES; we can’t find a solution unless we actually save these files temporarily. Since Wagtail sites mostly use wagtailimages and wagtaildocs instead, it seemed reasonable to put this aside for the moment.

@gasman gasman added this to the 1.10 milestone Mar 3, 2017

@BertrandBordage

This comment has been minimized.

Show comment
Hide comment
@BertrandBordage

BertrandBordage Mar 21, 2017

Member

Current Wagtail preview & how to break it

wagtail-old-preview

How it behaves using this pull request

wagtail-new-preview

Without WAGTAIL_AUTO_UPDATE_PREVIEW (default)

wagtail-preview-no-auto-update

With WAGTAIL_AUTO_UPDATE_PREVIEW = True

wagtail-preview-auto-update

Member

BertrandBordage commented Mar 21, 2017

Current Wagtail preview & how to break it

wagtail-old-preview

How it behaves using this pull request

wagtail-new-preview

Without WAGTAIL_AUTO_UPDATE_PREVIEW (default)

wagtail-preview-no-auto-update

With WAGTAIL_AUTO_UPDATE_PREVIEW = True

wagtail-preview-auto-update

@BertrandBordage

This comment has been minimized.

Show comment
Hide comment
@BertrandBordage

BertrandBordage Mar 21, 2017

Member

You can get the slides from the presentation I made today at Wagtail Space at this address: https://noripyt.com/en/wagtail-space-2017/
The video of the presentation is available here (first 15 minutes): https://www.youtube.com/watch?v=ObM2pUgY-bs

Member

BertrandBordage commented Mar 21, 2017

You can get the slides from the presentation I made today at Wagtail Space at this address: https://noripyt.com/en/wagtail-space-2017/
The video of the presentation is available here (first 15 minutes): https://www.youtube.com/watch?v=ObM2pUgY-bs

@thibaudcolas

Here are a couple of comments to make this more user-friendly (hopefully).

thisPreviewUrl = $this.data('action');
$icon.addClass('icon-spinner').removeClass('icon-view');
var previewWindow = window.open('', thisPreviewUrl);
previewWindow.focus();

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 22, 2017

Member

Not sure whether I like the idea of forcing focus on the preview window – presumably users opening a preview would expect to choose whether to look at it, or not?

@thibaudcolas

thibaudcolas Mar 22, 2017

Member

Not sure whether I like the idea of forcing focus on the preview window – presumably users opening a preview would expect to choose whether to look at it, or not?

This comment has been minimized.

@BertrandBordage

BertrandBordage Mar 23, 2017

Member

Most of my clients would be a bit lost if the preview doesn’t focus.
But this is a controversial subject, so you can see that it doesn’t work the same on Chrome & Firefox. One of them (I don’t remember which one) focus the tab when we open it but does nothing when we explicitly call .focus(), while the other one doesn’t focus on opening but accepts explicit focuses.

@BertrandBordage

BertrandBordage Mar 23, 2017

Member

Most of my clients would be a bit lost if the preview doesn’t focus.
But this is a controversial subject, so you can see that it doesn’t work the same on Chrome & Firefox. One of them (I don’t remember which one) focus the tab when we open it but does nothing when we explicitly call .focus(), while the other one doesn’t focus on opening but accepts explicit focuses.

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 24, 2017

Member

I guess the least confusing is then to follow the browser's behavior instead of explicitly calling .focus()? Would like to get others' opinion on this, I agree it's pretty controversial.

@thibaudcolas

thibaudcolas Mar 24, 2017

Member

I guess the least confusing is then to follow the browser's behavior instead of explicitly calling .focus()? Would like to get others' opinion on this, I agree it's pretty controversial.

This comment has been minimized.

@BertrandBordage

BertrandBordage Mar 24, 2017

Member

I had a chat with Olly from Torchbox, he’s in favour of keeping it like this, with an explicit focus.
I think we can keep it like this, and if people are annoyed by it and issues are open, we’ll simply reconsider.

@BertrandBordage

BertrandBordage Mar 24, 2017

Member

I had a chat with Olly from Torchbox, he’s in favour of keeping it like this, with an explicit focus.
I think we can keep it like this, and if people are annoyed by it and issues are open, we’ll simply reconsider.

This comment has been minimized.

@tomdyson

tomdyson Mar 24, 2017

Contributor

I think we should detail this in the release notes and encourage people to test this specific feature during the release candidate period. cc @ollywillans

@tomdyson

tomdyson Mar 24, 2017

Contributor

I think we should detail this in the release notes and encourage people to test this specific feature during the release candidate period. cc @ollywillans

then use preview again.
{% endblocktrans %}
</p>
<a class="button" href="javascript:window.close()">

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 22, 2017

Member

Could this JS be moved to a separate file?

@thibaudcolas

thibaudcolas Mar 22, 2017

Member

Could this JS be moved to a separate file?

This comment has been minimized.

@BertrandBordage

BertrandBordage Mar 23, 2017

Member

I know inline JS is considered a bad practice but… In this particular case, it’s not worth it in my opinion.
@gasman, what do you suggest?

@BertrandBordage

BertrandBordage Mar 23, 2017

Member

I know inline JS is considered a bad practice but… In this particular case, it’s not worth it in my opinion.
@gasman, what do you suggest?

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 24, 2017

Member

Ah sorry I didn't realise this was only for the "error" page, it's definitely overkill to have a separate file for that case.

@thibaudcolas

thibaudcolas Mar 24, 2017

Member

Ah sorry I didn't realise this was only for the "error" page, it's definitely overkill to have a separate file for that case.

$form.submit();
}
}).fail(function () {
alert('Error while sending preview data.');

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 22, 2017

Member

This needs to be a message directly on the page instead, with some styles / translations and suggestion on how to remediate the error.

@thibaudcolas

thibaudcolas Mar 22, 2017

Member

This needs to be a message directly on the page instead, with some styles / translations and suggestion on how to remediate the error.

This comment has been minimized.

@BertrandBordage

BertrandBordage Mar 23, 2017

Member

Where do you suggest we put that error message? If we put it on top of the page, like messages.error, it will not be visible if the user scrolled the page.

@BertrandBordage

BertrandBordage Mar 23, 2017

Member

Where do you suggest we put that error message? If we put it on top of the page, like messages.error, it will not be visible if the user scrolled the page.

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 24, 2017

Member

I would suggest to put it in a modal window, so it clearly looks dismissable and is always visible when the error happens. Ideally this should be a part of Wagtail's UI and used beyond this, but I'm not sure there is such a thing yet.

@thibaudcolas

thibaudcolas Mar 24, 2017

Member

I would suggest to put it in a modal window, so it clearly looks dismissable and is always visible when the error happens. Ideally this should be a part of Wagtail's UI and used beyond this, but I'm not sure there is such a thing yet.

This comment has been minimized.

@BertrandBordage

BertrandBordage Mar 24, 2017

Member

Hum… Alerts are also dismissible and they are also visible when the error occurs (except if you blocked them recently because too many of them were opened). I would also prefer adding a modal saying that an internal error occurred or users lost Internet connection, but I don’t think that’s easily possible to add one so… I’m a bit too lazy to write a view, template and static file just to display an unusual error more beautifully x)

@BertrandBordage

BertrandBordage Mar 24, 2017

Member

Hum… Alerts are also dismissible and they are also visible when the error occurs (except if you blocked them recently because too many of them were opened). I would also prefer adding a modal saying that an internal error occurred or users lost Internet connection, but I don’t think that’s easily possible to add one so… I’m a bit too lazy to write a view, template and static file just to display an unusual error more beautifully x)

@gasman gasman added this to Planned for 1.10 in Roadmap Mar 22, 2017

@BertrandBordage

This comment has been minimized.

Show comment
Hide comment
@BertrandBordage

BertrandBordage Mar 23, 2017

Member

Thanks @thibaudcolas for the review! I answered you in the code.

Member

BertrandBordage commented Mar 23, 2017

Thanks @thibaudcolas for the review! I answered you in the code.

@HvandenBerg

This comment has been minimized.

Show comment
Hide comment
@HvandenBerg

HvandenBerg Mar 24, 2017

Tested and OK
Tested cross browser and cross device in browserstack both with and without the auto update setting. Updated pages with images, headers, links and a few other rich text items. Tried both refreshing the preview page and clicking the preview button after editing the page to see updated page.

HvandenBerg commented Mar 24, 2017

Tested and OK
Tested cross browser and cross device in browserstack both with and without the auto update setting. Updated pages with images, headers, links and a few other rich text items. Tried both refreshing the preview page and clicking the preview button after editing the page to see updated page.

@BertrandBordage

This comment has been minimized.

Show comment
Hide comment
@BertrandBordage

BertrandBordage Mar 24, 2017

Member

Thanks @HvandenBerg! Glad it worked well for you!

Member

BertrandBordage commented Mar 24, 2017

Thanks @HvandenBerg! Glad it worked well for you!

@gasman gasman self-requested a review Mar 29, 2017

@kaedroho

Looks good to me! Just a few minor nitpicky things that I think would be nice.

Show outdated Hide outdated wagtail/wagtailadmin/static_src/wagtailadmin/js/page-editor.js
if ($previewButton.data('auto-update')) {
var setPreviewDataTimeout = -1;
// Form data is changed when field values are changed (change event),

This comment has been minimized.

@kaedroho

kaedroho Apr 12, 2017

Member

I think we should avoid sending any preview requests until the user clicks the preview button. Is that possible?

@kaedroho

kaedroho Apr 12, 2017

Member

I think we should avoid sending any preview requests until the user clicks the preview button. Is that possible?

This comment has been minimized.

@BertrandBordage

BertrandBordage Apr 12, 2017

Member

That’s right, I thought I did it, I must have forgotten. I’ll change it this afternoon.

@BertrandBordage

BertrandBordage Apr 12, 2017

Member

That’s right, I thought I did it, I must have forgotten. I’ll change it this afternoon.

form = form_class(request.POST, request.FILES, instance=page, parent_page=parent_page)
class PreviewOnEdit(View):
http_method_names = ('post', 'get')
preview_expiration_timeout = 60 * 60 * 24 # seconds

This comment has been minimized.

@kaedroho

kaedroho Apr 12, 2017

Member

Do previews need to be kept for a day? Wouldn't a timeout of 1 minute (or even 3 seconds!) do?

@kaedroho

kaedroho Apr 12, 2017

Member

Do previews need to be kept for a day? Wouldn't a timeout of 1 minute (or even 3 seconds!) do?

This comment has been minimized.

@BertrandBordage

BertrandBordage Apr 12, 2017

Member

It’s not really important, in fact. Every time the preview for a page is updated, the previous preview data is removed. That timeout is used to delete old previews for other pages. If you create a preview for page 1, wait 10 minutes, work on page 2 and make a preview of it, maybe you kept preview for page 1 in a tab. So it’s better to keep it for at least a few hours in my opinion. When you create a preview for page 2 more than 24 hours after a page 1 preview, then data from this preview 1 is deleted.

@BertrandBordage

BertrandBordage Apr 12, 2017

Member

It’s not really important, in fact. Every time the preview for a page is updated, the previous preview data is removed. That timeout is used to delete old previews for other pages. If you create a preview for page 1, wait 10 minutes, work on page 2 and make a preview of it, maybe you kept preview for page 1 in a tab. So it’s better to keep it for at least a few hours in my opinion. When you create a preview for page 2 more than 24 hours after a page 1 preview, then data from this preview 1 is deleted.

Show outdated Hide outdated wagtail/wagtailadmin/views/pages.py
def remove_old_preview_data(self):
expiration = time() - self.preview_expiration_timeout
for k, v in self.request.session.items():
if not k.startswith('preview-'):

This comment has been minimized.

@kaedroho

kaedroho Apr 12, 2017

Member

Maybe we should prefix keys with wagtail-preview-. Just in case some other project uses them/copies this code :)

@kaedroho

kaedroho Apr 12, 2017

Member

Maybe we should prefix keys with wagtail-preview-. Just in case some other project uses them/copies this code :)

This comment has been minimized.

@BertrandBordage

BertrandBordage Apr 12, 2017

Member

Right, I’ll change it this afternoon.

@BertrandBordage

BertrandBordage Apr 12, 2017

Member

Right, I’ll change it this afternoon.

@BertrandBordage

This comment has been minimized.

Show comment
Hide comment
@BertrandBordage

BertrandBordage Apr 12, 2017

Member

Thanks a lot for the review, Karl! I’ll do the changes within a few hours.

Member

BertrandBordage commented Apr 12, 2017

Thanks a lot for the review, Karl! I’ll do the changes within a few hours.

@BertrandBordage

This comment has been minimized.

Show comment
Hide comment
@BertrandBordage

BertrandBordage Apr 12, 2017

Member

I applied the changes.

Member

BertrandBordage commented Apr 12, 2017

I applied the changes.

@gasman

gasman approved these changes Apr 12, 2017

@kaedroho kaedroho merged commit 511c158 into wagtail:master Apr 12, 2017

2 of 3 checks passed

continuous-integration/drone/pr the build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tomdyson tomdyson moved this from Planned for 1.10 to In 1.10 in Roadmap Apr 12, 2017

@BertrandBordage BertrandBordage deleted the BertrandBordage:new-admin-preview branch Apr 19, 2017

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