Code development platform for open source projects from the European Union institutions

Skip to content
Snippets Groups Projects

OEL-1653: Show time in event dates

Merged Francesco SARDARA requested to merge OEL-1653 into 1.x

Created by: escuriola

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 827819fc
  • 54 54 oe_content_short_title: true
    55 55 oe_documents: true
    56 56 oe_featured_media: true
    57 search_api_excerpt: true
    • Created by: donquixote

      And another thing I forgot to mention. We are changing the appearance of date range teaser, but we are not changing the formatter or showing the time. Do we actually have a spec for this?

    • Created by: escuriola

      The Event teaser uses short dates in the PDF, but instead of "to" the separator is "-". For this reason I didn't change anything.

    • Created by: donquixote

      I do see the "Event teaser" part in the pdf now. It does use hyphen with spaces. I see that we don't need to add the spaces with the separator here, the core formatter already adds the spaces. So it seems ok.

      Can we add a line in ContentEventRenderTest::testEventRenderingTeaser() to test the date output? I think we currently don't have it.

  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 56edcbfe
  • 1 langcode: en
    2 status: true
    3 dependencies: { }
    4 id: oe_whitelabel_date_time_long
    5 label: 'Date and time, long names'
    6 default_pattern: 'l d F Y, H.i (T)'
    7 default_separator: ' - '
    8 same_day_start_pattern: 'l d F Y, H.i'
    9 same_day_end_pattern: 'H.i (T)'
    10 same_day_separator: '-'
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 56edcbfe
  • 9 9 "php": ">=7.4",
    10 10 "cweagans/composer-patches": "^1.7",
    11 11 "drupal/core": "^9.2",
    12 "drupal/daterange_compact": "^2.0",
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 56edcbfe
  • 130 80 trim($content_banner->filter('.oe-sc-event__oe-summary')->text())
    131 81 );
    132 82
    83 $date = $crawler->filter('dl > dd');
    84
    85 // Assert event dates starting and ending same day.
    86 $this->assertEquals('Wednesday 09 February 2022, 21.00-23.00 (CET)', trim($date->text()));
    87
    88 // Assert event dates starting and ending at different days.
    • Created by: donquixote

      These "Assert" comments feel wrong to me, if they are not on top of an actual assertion. Here we are just setting things up, the assert happens later. ("arrange, act, assert")

    • Created by: brummbar

      That's to explain what is this block of code doing.

  • Francesco SARDARA
  • Created by: donquixote

    Review: Changes requested

    A bit of feedback.

    And one question: Should we have tests for the date range formats in isolation, or is it enough to test them as part of a content type? If we test in isolation we could also cover translatability.

    But not a blocker, I would be ok to merge this without these additional tests. The event test will reveal regressions in the date range formats.

  • Created by: donquixote

    Review: Approved

  • Created by: drishu

    Review: Approved

  • Merged by: brummbar at 2022-06-29 16:48:34 UTC

  • merged manually

  • Please register or sign in to reply
    Loading