Problem/Motivation

Hi!

After the field_type_category API has landed, core/core.field_type_category.yml gefines the category name "date_time" for some fields.

Then, both:
- TimestampItem.php (Drupal\Core\Field\Plugin\Field\FieldType),
- DateTimeItem.php (datetime\Plugin\Field\FieldType),
- and DateRangeItem.php (datetime_range\Plugin\Field\FieldType)
in their annotation it is used category = "date_time",.

However, in the module datetime_range, in its .module file we find:

/**
 * Implements hook_field_type_category_info_alter().
 */
function datetime_range_field_type_category_info_alter(&$definitions) {
  // The `datetime_range` field type belongs in the `general` category, so the
  // libraries need to be attached using an alter hook.
  $definitions[FieldTypeCategoryManagerInterface::FALLBACK_CATEGORY]['libraries'][] = 'datetime_range/drupal.datetime_range-icon';
}

using the wrong category and it should be 'date_time' instead, so the whole hook is not needed and we can remove it (commenting it does not make any impact in the 'add field' page of any content type) from the .module file and also the library and css involved as they are not used anymore.

In addition, if we take a look at FieldStorageAddForm() (line 346), we have:

    $form['#attached']['library'] = [
      'field_ui/drupal.field_ui',
      'field_ui/drupal.field_ui.manage_fields',
      'core/drupal.ajax',
    ];

and in the library 'field_ui/drupal.field_ui.manage_fields that calls field_ui/css/field_ui.icons.theme.css we can find the proper css that is used for the fields that are grouped under the category 'date_time':

.field-icon-date_time {}

In the same file, we find as well

.field-icon-daterange {}

but it is not used anymore as 'date_time' is the category in all the fields and can be removed too.

Steps to reproduce

Comment the whole function in the .module, then CR and see the icon is still there (in the "add new field" page under any CT bundle admin/structure/types/manage/<bundle>/fields/add-field); therefore any mention to 'daterange' css icon is not used.

Proposed resolution

- Remove the datetime_range_field_type_category_info_alter() on the .module file
- Remove the datetime_range.libraries.yml file as it it might not be needed.
- Remove the css folder, as the files inside are not needed (duplicated)
- Update the Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTest class and remove datetime_range/drupal.datetime_range-icon from there.
- Remove the .field-icon-daterange from field_ui/css/field_ui.icons.theme.css as it is not used either.

Best

Issue fork drupal-3409663

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

gorkagr created an issue. See original summary.

cilefen’s picture

SandeepSingh199 made their first commit to this issue’s fork.

sandeepsingh199’s picture

Status: Active » Needs review
gorkagr’s picture

Status: Needs review » Needs work

Hi!
You just changed the description but not the actual array value.

Edit:
Actually, it seems the whole hook function could be removed as it seems the icon and the class are duplicated and they are already added (you can check this by commenting the whole function datetime_range_field_type_category_info_alter() )

In FieldStorageAddForm() (line 346), we have:

    $form['#attached']['library'] = [
      'field_ui/drupal.field_ui',
      'field_ui/drupal.field_ui.manage_fields',
      'core/drupal.ajax',
    ];

and in the library 'field_ui/drupal.field_ui.manage_fields that calls field_ui/css/field_ui.icons.theme.css we can find the same

.field-icon-daterange {}

as in the datetime_range.icon.theme.css

So, the proposal could be:
- Remove the datetime_range_field_type_category_info_alter() on the .module file
- Remove the .libraries file as it it might not be needed.
- Remove the css folder, as they are not needed
- Update the Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTest class and remove datetime_range/drupal.datetime_range-icon from there.

gorkagr’s picture

Title: datetime_range field type belongs to wrong category? » Module datetime_range have duplicated functionalities and styles
Issue summary: View changes
gorkagr’s picture

Issue summary: View changes
gorkagr’s picture

Issue summary: View changes
gorkagr’s picture

Hi, sorry for all the comment and edits

I have also noticed the css it is used for rendering the icon field-icon-date_time, not even field-icon-daterange, as it takes the category for the rendering due to the new panel.

sakthi_dev made their first commit to this issue’s fork.

sakthi_dev’s picture

Hi @gorkagr, updated the code based on comment #6. Should I change the array value or should it stay with 6th comment.

gorkagr’s picture

That is more a question for a core maintainer

gorkagr’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

MR Seems to have some failures.

gorkagr’s picture

Status: Needs work » Needs review
smustgrave’s picture

Version: 10.2.x-dev » 11.x-dev
Status: Needs review » Needs work

MR should be updated 11.x as the current development branch.

Looking at it though, I think it's the opposite and css should be removed from field_ui to be consistent with how other contrib modules have theirs. But since range is now under date_time the icon doesn't appear regardless so icon should be removed through out.

sakthi_dev’s picture

Status: Needs work » Needs review

Rebased with 11.x and also removed the daterange icon from field_ui and also removed the icon. Please review.

gorkagr changed the visibility of the branch 3409663-rebased-datetime-duplication to hidden.

gorkagr changed the visibility of the branch 3409663-rebased-datetime-duplication to active.

smustgrave’s picture

Title: Module datetime_range have duplicated functionalities and styles » Remove datetime_range icon and library
Status: Needs review » Reviewed & tested by the community

Removal seems complete.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

We seem to be removing the CSS from both the field ui and the datetime range modules in the latest MR.

That doesn't seem right.

Can we update the issue summary and remove the old approach/EDIT comments so that there's a clear statement of the issue and what the proposed resolution is.

Thanks

gorkagr’s picture

Issue summary: View changes
gorkagr’s picture

Status: Needs work » Needs review

IS updated

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Issue summary has been updated. Essentially this is dead code now as datarange isn't it's own category anymore.

larowlan’s picture

Issue tags: +Bug Smash Initiative

Issue credits

  • larowlan committed aa55822a on 10.2.x
    Issue #3409663 by sakthi_dev, gorkagr, smustgrave: Remove datetime_range...

  • larowlan committed b90102de on 11.x
    Issue #3409663 by sakthi_dev, gorkagr, smustgrave: Remove datetime_range...
larowlan’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and backported to 10.2.x to match #3401464: Date range should be in the date_time category

Thanks folks

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.