Replace using BroadcastChannel by injecting error message into error templates #104
Conversation
| }; | ||
|
|
||
| let body = text.replace( /<!--WP_SERVICE_WORKER_ERROR_MESSAGE-->/, errorMessages.error ); | ||
| body = body.replace( /<!--WP_SERVICE_WORKER_ERROR_DETAILS-->/, errorText ); |
There was a problem hiding this comment.
@westonruter Should we have a placeholder text here in case there are no details for the error? For example No details received for the error or something similar?
There was a problem hiding this comment.
That's a good question. I don't think so, since if the details element is used then it would be expanded to find no additional details. The problem seems to be that the container element needs to be omitted if there is no error message, right?
There was a problem hiding this comment.
Maybe, I had the thought that for consistency it would always be good to display the details even if there are no details -- to state explicitly there are no details so that it would be clear there were no details. Otherwise perhaps the user wouldn't know that no details were found.
If you think it's better to just not display the details at all then we can do that instead. Thoughts?
There was a problem hiding this comment.
Maybe better for now to just omit the details entirely if there is no error message detected.
But I see that this presumes that the details isn't already present in the document.
| return caches.match( ERROR_500_BODY_FRAGMENT_URL ); | ||
| } | ||
|
|
||
| return response.text().then( function( errorText ) { |
There was a problem hiding this comment.
This part is duplicated in the offline commenting section.
miina
changed the title
wp-includes/theme-compat/500.php
Outdated
| } | ||
| </script> | ||
| <?php wp_print_service_worker_error_details_script( 'renderErrorDetails' ); ?> | ||
| <iframe style="width:100%;" srcdoc="<?php wp_service_worker_error_details_placeholder(); ?>"></iframe> |
There was a problem hiding this comment.
The wp_service_worker_error_details_placeholder() won't work so well here because if trying to access the error page directly then you'd get:
<iframe style="width:100%;" srcdoc="<!--WP_SERVICE_WORKER_ERROR_DETAILS-->"></iframe>That seems like it would be an HTML parse error for some parsers.
| channel.close(); | ||
| }, 30 * 1000 ); | ||
| let body = text.replace( /<!--WP_SERVICE_WORKER_ERROR_MESSAGE-->/, errorMessages.error ); | ||
| body = body.replace( /<!--WP_SERVICE_WORKER_ERROR_DETAILS-->/, errorText ); |
There was a problem hiding this comment.
I believe this would need to get encoded in order to be able to appear in an HTML attribute. If the errorText contains markup (as it is expected to) then it will likely break the srcdoc attribute.
wp-includes/theme-compat/500.php
Outdated
| } | ||
| </script> | ||
| <?php wp_print_service_worker_error_details_script( 'renderErrorDetails' ); ?> | ||
| <iframe style="width:100%;" srcdoc="<?php wp_service_worker_error_details_placeholder(); ?>"></iframe> |
There was a problem hiding this comment.
Something else to consider is that srcdoc is not supported by all browsers. So perhaps it would make more sense to encode the errorText as an HTML data URI and then populate the src instead.
| }; | ||
|
|
||
| let body = text.replace( /<!--WP_SERVICE_WORKER_ERROR_MESSAGE-->/, errorMessages.error ); | ||
| body = body.replace( /<!--WP_SERVICE_WORKER_ERROR_DETAILS-->/, errorText ); |
There was a problem hiding this comment.
Maybe better for now to just omit the details entirely if there is no error message detected.
But I see that this presumes that the details isn't already present in the document.
westonruter
changed the title
|
I've updated the theme to use these updated functions: westonruter/twentyseventeen-westonson#14 |
Fixes #86.
Adds placeholder tag for error details and replaces using BroadcastChanel.