Support upload and display of new images #800
Conversation
e438f91 to
7a78bcd
Compare
8193d51 to
6c38824
Compare
6c38824 to
24576bf
Compare
24576bf to
dbed127
Compare
623d692 to
f45ce9d
Compare
dbed127 to
be6ba79
Compare
be6ba79 to
9018a44
Compare
9018a44 to
6fc944e
Compare
90a1c2d to
0b2d518
Compare
6fc944e to
ae311e1
Compare
0b2d518 to
863f432
Compare
ae311e1 to
fe20c0d
Compare
863f432 to
cbf8f0d
Compare
a2322af to
2405034
Compare
|
@kevindew this is now ready, when you get a moment :-). |
2405034 to
ecef905
Compare
ecef905 to
7518656
Compare
7518656 to
7070f3b
Compare
7070f3b to
0f8b136
Compare
| @@ -50,6 +75,17 @@ InlineImageModal.prototype.performAction = function (item) { | |||
| var editor = this.$module.closest('[data-module="markdown-editor"]') | |||
| this.$modal.close() | |||
| editor.selectionReplace(item.dataset.modalData) | |||
| }, | |||
| 'upload': function () { | |||
| this.postModalForm(item.dataset.modalActionForm) | |||
There was a problem hiding this comment.
Are we vulnerable at this point to dual submissions? I assume that nothing changes in the UI once they submit the form until we receive a response which could be a while for an upload.
Could well be that we have the same problem with the non modal form of this too but feedback seems to be clearer in a browser when you're changing a whole page.
There was a problem hiding this comment.
So we are actually changing the modal to the loading screen - this happens before any action is invoked, as otherwise it kind of defeats the point of the loading screen ;-). I think there's a separate question, which you also asked, about the behaviour of short-lived loading screens.
I guess they can potentially click the button twice, really quickly, although I can't do this myself - in a very slow browser, perhaps.
There was a problem hiding this comment.
Oh ok - I think I might just not be understanding how this works. Is this correct?
- When I click the button to submit the form it prevents default on that event
- It focuses the modal again
- It hides the form and shows the loading view
- it pulls the data from the hidden view and submits it
- that form is only cleared once showDynamic section is called again?
There was a problem hiding this comment.
Yes, that's right - is there still something you're concerned about here?
There was a problem hiding this comment.
Nope - just taking my old brain a while to understand
app/views/images/index.html.erb
Outdated
| create_image_path(@document), | ||
| id: "image-upload", | ||
| multipart: true, | ||
| data: { gtm: "image-upload-submit" }, |
There was a problem hiding this comment.
Do you know if GTM will still receive this event? I assume this listens to form submit events but it doesn't look like we listen to any of those.
There was a problem hiding this comment.
@alex-ju is currently working on rewriting our local modules/ to support re-initialisation, just like components. I think you're referring to GTMFormListener here, so it should be fixed by that :-).
There was a problem hiding this comment.
I was referring to lack of a submit event existing. If GTM is listening for form submit events then this one will never be called because we preventDefault on the button click event.
GTM likely has all sorts of voodoo though so this may not be remotely a problem.
There was a problem hiding this comment.
You're right - it wouldn't have worked. I've just added a commit that, with the work @alex-ju is doing, will preserve the current behaviour.
This exposes the upload form in the modal context of the images index page, and overrides the 'Upload' button to post the form data i.e. upload the image. Depending on the form data, we may need to render the response HTML to display requirements issues, or follow a redirect. The API of the FormData class means we can support posting a form generically, which is handled using a new 'postModalForm' function in the existing 'glue' module called 'inline-image-modal'. We also follow any redirects automatically, to avoid unnecessary boilerplate '.fetch's. In future work we will implement the rest of the upload workflow, so that the modal loads the crop page after the image is uploaded.
0f8b136 to
1800d6e
Compare
1800d6e to
3f09a8c
Compare
3f09a8c to
a6bfa7a
Compare
This manually focusses any error-summary component to simulate its normal behaviour. Ideally, we should change this and other components in govuk-frontend so that they can be re-initialized, and in this case, stop relying on window events to auto-focus. We also have components from govuk_publishing_components, which can be idempotently re-initialized with 'GOVUK.modules.start()', but this is not necessary yet. And we have our local components, which don't support re-initialization at all, which we will address in future work.
a6bfa7a to
e3c7903
Compare
|
Glad the submit thing wasn't much of a pain. Thinking about it it would be pretty trivial to support form submissions too if we had listen on submit events as well and set data on form rather than buttons. |
| end | ||
|
|
||
| def and_i_upload_an_invalid_image | ||
| find('form input[type="file"]').set(Rails.root.join(file_fixture("text-file.txt"))) |
There was a problem hiding this comment.
I wrote a comment on this before but it doesn't seem to be here so apologies if it's come through twice - I'm assuming I just lost it in the review
I notice we didn't need the Rails.root.join on
so wondering if this is needed or special.There was a problem hiding this comment.
Yes, it was strange - I encountered this and forgot to come back to it. I've added another commit to explain, but it seems like absolute paths are required for JS feature specs.
There was a problem hiding this comment.
Oh that's a pain - thanks for the commit explanation
Previously we overrode the behaviour of the 'Upload' button to support the inline image workflow, but this interferes with our GTM 'submit' listener. This changes the way we override the 'Upload' action, so that it happens on the form instead of the button - with some changes to our GTMFormListener module to support re-initialisation, this works.
This is only required for the JS feature specs, where the headless engine seems to need absolute paths in order to work.
https://trello.com/c/QDEuNPgp/653-allow-users-to-upload-an-inline-image-in-the-context-of-a-modal
This exposes the upload form in the modal context of the images index
page, and overrides the 'Upload' button to post the form data i.e. upload
the image. Depending on the form data, we may need to render the
response HTML to display requirements issues, or follow a redirect.
The API of the FormData class means we can support posting a form
generically, which is handled using a new 'postModalForm' function in
the existing 'glue' module called 'inline-image-modal'. We also follow
any redirects automatically, to avoid unnecessary boilerplate '.fetch's.
In future work we will implement the rest of the upload workflow, so
that the modal loads the crop page after the image is uploaded.