Created an MR from the patch
Closes #2614950
Merge request reports
Activity
28 28 */ 29 29 const BLOCK_LABEL_VISIBLE = 'visible'; 30 30 31 /** 32 * Indicates the block label (title) should be hidden from end users. 33 * 34 * @todo This value should be changed to "hidden" for consistency. 35 * See https://www.drupal.org/node/2863313. 36 */ 37 const BLOCK_LABEL_HIDDEN = '0'; 38 39 /** 40 * Indicates the block label (title) should be visually hidden. 41 */ 42 const BLOCK_LABEL_VISUALLY_HIDDEN = 'visually_hidden'; - Comment on lines 28 to +42
So we have an existing
const BLOCK_LABEL_VISIBLE = 'visible';, then introducing aBLOCK_LABEL_HIDDENwith a numeric value andBLOCK_LABEL_VISUALLY_HIDDEN = 'visually_hidden';with a string value.This seems like more values than needed for only three possibilities (label rendered or not + visually hidden if rendered) Is there a way to introduce settings with less new constants and in a way with more consistency with the value names?
Yes i think we can use
const BLOCK_LABEL_HIDDEN = false;instead of the two constants introduced here. By adopting this approach, we can reduce the complexity of constants while maintaining clarity and consistency in how block labels are managed in terms of visibility and accessibility. Also keeping the variable as boolean will maintain more consistency instead of keeping it as a numeric value.
443 443 label_display: 444 444 type: string 445 445 label: 'Display title' 446 label_display_type: 447 type: string 448 label: 'Visually hidden label' -
I have a few suggestions for this.Please let me know which one is the best amongst:-
- label_visibility_options
- label_hidding_options
- label_accessibility.
Edited by utkarsh_33
200 187 201 // Add plugin-specific settings for this block type. 188 202 $form += $this->blockForm($form, $form_state); 189 203 return $form; 190 204 } 191 205 206 /** 207 * Returns an array of visibility options for the block label (title). 208 * 209 * @return array 210 * An array of visibility options. 211 */ 212 protected function getLabelDisplayOptions() { 213 return [ 214 BlockPluginInterface::BLOCK_LABEL_HIDDEN => $this->t('Hidden'), 215 BlockPluginInterface::BLOCK_LABEL_VISUALLY_HIDDEN => $this->t('Visually Hidden'), -
A few better options:-
- Hidden from view.
- Hidden, Readable by Screen.
I could not think of much options which are short and descriptive as well for this.In my opinion the 1st option could be better replacement option for the existing one. Please suggest if anyone has a better suggestion.Thanks!
Edited by utkarsh_33
79 80 '#default_value' => TRUE, 80 81 '#return_value' => 'visible', 81 82 ], 83 'label_display_type' => [ 84 '#type' => 'select', 85 '#title' => 'Hide method', 86 '#description' => 'Method of hiding the block title.', - Comment on lines +85 to +86
changed this line in version 4 of the diff
38 39 * {@inheritdoc} 39 40 */ 40 41 public function defaultConfiguration() { 41 return ['label_display' => FALSE]; 42 return [ 43 'label_display' => FALSE, 44 'label_display_type' => BlockPluginInterface::BLOCK_LABEL_HIDDEN, -
By making the default value
BlockPluginInterface::BLOCK_LABEL_HIDDEN, this means users have to opt-in to making the labels accessible. The accessible visually-hidden version should be the default. There are plenty of places that Drupal already does thisThe only reason IMO that this is even a setting is that existing sites might be disrupted by the unexpected addition of the visually hidden titles. Whatever the config schema winds up being, it should be set up so existing sites render the block titles the same way, but any new installs should default to having the hidden blocks be visually-hidden.
249 249 $variables['base_plugin_id'] = $variables['elements']['#base_plugin_id']; 250 250 $variables['derivative_plugin_id'] = $variables['elements']['#derivative_plugin_id']; 251 251 $variables['in_preview'] = $variables['elements']['#in_preview'] ?? FALSE; 252 $variables['label'] = !empty($variables['configuration']['label_display']) ? $variables['configuration']['label'] : ''; 252 $is_label_visible = ($variables['configuration']['label_display'] ?? BlockPluginInterface::BLOCK_LABEL_HIDDEN) == BlockPluginInterface::BLOCK_LABEL_VISIBLE; 253 $is_label_visually_hidden = !$is_label_visible && ($variables['configuration']['label_display_type'] ?? BlockPluginInterface::BLOCK_LABEL_HIDDEN) == BlockPluginInterface::BLOCK_LABEL_VISUALLY_HIDDEN; 254 $variables['label'] = isset($variables['configuration']['label']) ? $variables['configuration']['label'] : '';