Repository navigation
Added “Display on“ options to Map element - #315
Conversation
a334f6d to
3c9dffb
Compare
|
@rimi-itk jeg kan se at du har bedt Stan om at lave review på denne opgave, men jeg kan ikke se noget issue omkring det PR, så jeg ved ikke helt hvem der har bestilt opgaven og hvem der skal betale for review. Kan du gøre mig klogere? |
|
@ds-bellcom, jeg har tilføjet en henvisning til et tidligere pull request (og til Leantime) i beskrivelsen. |
|
@OS2Forms/koordinationsgruppe Inden vi kan planlægge at lave review på denne opgave, så vil jeg gerne have en godkendelse på opgaven. Jeg vil forvente at det kan klares på ca. 2 timer. Er det OK? |
@OS2Forms/koordinationsgruppe kan de 2 timer godkendes? |
| $viewMode = 'pdf'; | ||
| } | ||
|
|
||
| // @todo Is this (i.e. $element['#display_image_on']) really the way to get element configuration? |
There was a problem hiding this comment.
did you forget to remove this TODO comment or it's left intentionally?
| } | ||
|
|
||
| // @todo Is this (i.e. $element['#display_image_on']) really the way to get element configuration? | ||
| $showImageOn = array_filter((array) ($element['#display_image_on'] ?? NULL)); |
There was a problem hiding this comment.
Existing map elements do not have #display_image_on checks resolve to false and the formatter returns an empty array. Since this PR also removes the previous template, existing submissions lose their map output after upgrading
There was a problem hiding this comment.
The map functionality has been broken since the Drupal 10 upgrade. Therefore there is no backwards compatibility.
| $showImageOn = array_filter((array) ($element['#display_image_on'] ?? NULL)); | ||
| $includeImage = isset($showImageOn[$viewMode]); | ||
|
|
||
| $showGeoJsonOn = array_filter((array) ($element['#display_geojson_on'] ?? NULL)); |
There was a problem hiding this comment.
Existing map elements do not have #display_geojson_on, so both checks resolve to false and the formatter returns an empty array. Since this PR also removes the previous template, existing submissions lose their map output after upgrading
There was a problem hiding this comment.
The map functionality has been broken since the Drupal 10 upgrade. Therefore there is no backwards compatibility.
|
|
||
| // @todo Is this (i.e. $element['#display_image_on']) really the way to get element configuration? | ||
| $showImageOn = array_filter((array) ($element['#display_image_on'] ?? NULL)); | ||
| $includeImage = isset($showImageOn[$viewMode]); |
There was a problem hiding this comment.
we need to check selected values with in_array() rather than isset(): the defaults are numerically indexed lists, so checking keys such as html also fails.
There was a problem hiding this comment.
Switched to in_array() in combination with getElementProperty in fe4c04c.
| $includeImage = isset($showImageOn[$viewMode]); | ||
|
|
||
| $showGeoJsonOn = array_filter((array) ($element['#display_geojson_on'] ?? NULL)); | ||
| $includeGeoJson = isset($showGeoJsonOn[$viewMode]); |
There was a problem hiding this comment.
we need to check selected values with in_array() rather than isset(): the defaults are numerically indexed lists, so checking keys such as html also fails.
|
|
||
| $viewMode = $options['view_mode'] ?? 'results'; | ||
| if ('table' === $viewMode) { | ||
| $viewMode = 'html'; |
There was a problem hiding this comment.
When a PDF uses the table layout, the options look like this
$options = [
'view_mode' => 'table',
'pdf' => TRUE,
];
Current version of the could would treats it as HTML and skips the PDF check. This means the PDF follows the HTML display settings instead of the PDF settings.
For example, an image enabled only for PDF will be missing from a PDF with a table layout. Please check the PDF/email flags before handling the table layout.
It's ok, if that's intentional though
| $value = $this->getMapValue($element, $webform_submission, $options); | ||
|
|
||
| return $value['geojson']; | ||
| } |
There was a problem hiding this comment.
formatTextItem() always returns GeoJSON without checking #display_geojson_on.
Unchecking “Text”, or disabling GeoJSON everywhere, therefore has no effect on text output. Please apply the configured display rules here as well, including the email context for plain-text emails.
There was a problem hiding this comment.
Added missing configuration display checks in fe4c04c!
|
Please wait to merge this PR until the Drupal 11 release is out. |
|
@jekuaitk this PR may be merged to develop-branch when the corrections is made. |
Closes #335
This is a follow-up on #289 (and hence related to https://os2forms-leantime.itkdev.dk/_#/tickets/showTicket/180).
Adds “Display … on” settings to Map element to control where the image and the GeoJSON are displayed:
Along the way the template/theme hack to display the image has been removed.