Fix showing end dates when meetings have multiple dates#16288
Merged
Fix showing end dates when meetings have multiple dates#16288
Conversation
Contributor
📝 WalkthroughWalkthroughReworks meeting calendar markup and styles to conditionally render end month/day only when different from start; adds helpers to detect same month/day; adjusts calendar width and spacing; updates tests to cover single-day, multi-day same-month, multi-month, and no-end-time cases. Changes
Sequence Diagram(s)(Skipped — changes are view/markup and styling with small helper predicates; no multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb (1)
65-107: Please add a cross-year date-range regression spec.Current scenarios cover same-month and cross-month in the same year, but not a range like
2020-10-15→2021-10-17. Adding it will lock the intended behavior for year boundaries.Suggested spec addition
+ context "when meeting spans the same month across different years" do + let!(:meeting) { create(:meeting, :published, start_time: Time.new(2020, 10, 15, 10, 0, 0, 0), end_time: Time.new(2021, 10, 17, 12, 0, 0, 0)) } + + it "shows the range instead of collapsing into a single date" do + expect(subject).to have_css(".card__calendar--wide") + expect(subject).to have_css(".card__calendar-day", text: "15") + expect(subject).to have_css(".card__calendar-day", text: "17") + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb` around lines 65 - 107, Add a new spec context in meeting_l_cell_spec.rb named something like "when meeting spans years" that creates a published meeting with start_time Time.new(2020,10,15,10,0,0) and end_time Time.new(2021,10,17,12,0,0) (use the same let!(:meeting) pattern), then assert via subject that the start month and end month are rendered (expect(subject).to have_css(".card__calendar-month", text: "Oct") for both start and end), that start and end days render (".card__calendar-day" with "15" and "17"), and that the view shows the month separator (".meeting__calendar-separator") and the wide calendar class (".card__calendar--wide"); use the same selectors and test style as the existing cross-month context so the regression locks cross-year behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@decidim-meetings/app/cells/decidim/meetings/meeting_l_cell.rb`:
- Around line 39-49: The same_month? and same_day? methods compare only
month/day and therefore misclassify meetings spanning year boundaries; update
both methods (same_month? and same_day?) to compare full dates or include year
in the comparison (e.g., compare meeting.start_time.to_date ==
meeting.end_time.to_date for day, or compare start_time.year == end_time.year &&
start_time.month == end_time.month for month) while preserving the existing
early-return when meeting.end_time.blank?. Ensure you reference
meeting.start_time and meeting.end_time in the new comparisons.
In `@decidim-meetings/app/cells/decidim/meetings/meeting_l/image.erb`:
- Around line 5-13: In meeting_l/image.erb the separator <p> uses two different
CSS classes (meeting__calendar-separator and card__calendar-separator), causing
inconsistent styling; standardize them by changing the first separator's class
(the <p> between the month outputs that currently reads
meeting__calendar-separator) to card__calendar-separator so both separators use
the same class in this card__calendar template.
---
Nitpick comments:
In `@decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb`:
- Around line 65-107: Add a new spec context in meeting_l_cell_spec.rb named
something like "when meeting spans years" that creates a published meeting with
start_time Time.new(2020,10,15,10,0,0) and end_time Time.new(2021,10,17,12,0,0)
(use the same let!(:meeting) pattern), then assert via subject that the start
month and end month are rendered (expect(subject).to
have_css(".card__calendar-month", text: "Oct") for both start and end), that
start and end days render (".card__calendar-day" with "15" and "17"), and that
the view shows the month separator (".meeting__calendar-separator") and the wide
calendar class (".card__calendar--wide"); use the same selectors and test style
as the existing cross-month context so the regression locks cross-year behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
decidim-core/app/packs/stylesheets/decidim/_cards.scssdecidim-meetings/app/cells/decidim/meetings/dates_and_map/show.erbdecidim-meetings/app/cells/decidim/meetings/meeting_l/image.erbdecidim-meetings/app/cells/decidim/meetings/meeting_l_cell.rbdecidim-meetings/app/packs/stylesheets/decidim/meetings/_item.scssdecidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb
decidim-meetings/app/cells/decidim/meetings/meeting_l/image.erb
Outdated
Show resolved
Hide resolved
c91e91b
7678f6d
7678f6d to
61adc64
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb (1)
92-94:⚠️ Potential issue | 🟠 MajorFix selector mismatch in the no-end-time separator assertion
On Line 93, the spec checks
.meeting__calendar-separator, but the card template uses.card__calendar-separator. This can let regressions pass undetected.Proposed fix
- it "does not show separator" do - expect(subject).to have_no_css(".meeting__calendar-separator") - end + it "does not show separator" do + expect(subject).to have_no_css(".card__calendar-separator") + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb` around lines 92 - 94, The spec's expectation in the "it does not show separator" example is using the wrong CSS selector; update the assertion in the example (the block under it "does not show separator" that calls expect(subject).to have_no_css(...)) to check for ".card__calendar-separator" instead of ".meeting__calendar-separator" so it matches the card template's class; keep the rest of the test (subject, example name) unchanged.
🧹 Nitpick comments (2)
decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb (1)
52-54: Make the “no month separator” assertion explicitThe example title says no month separator, but it only checks month text. Add a scoped absence check to match the intent.
Proposed refinement
it "does not show month separator" do expect(subject).to have_css(".card__calendar-month", text: "October") + expect(subject).to have_no_css(".card__calendar-month .card__calendar-separator") end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb` around lines 52 - 54, The spec example "it 'does not show month separator'" currently only asserts the month text; update the example to explicitly assert the absence of the month-separator element by adding a scoped negative matcher against the separator element (for example: expect(subject).not_to have_css(".card__calendar-month + .card__calendar-month-separator") or expect(subject).not_to have_css(".card__calendar-month-separator") alongside the existing expect(subject).to have_css(".card__calendar-month", text: "October") so the test both confirms the month is present and that no separator node is rendered.decidim-meetings/app/cells/decidim/meetings/meeting_l/image.erb (1)
2-18: Align with design system: replace<p>with<span>inside calendar componentsThe design system mockup uses
<span>elements for all calendar content, but this template nests<p>inside<span>containers, which violates HTML content models. Update to use<span>throughout for consistency with the design system and correct semantic structure.Note: The CSS rule
empty:[&>p]:hiddenin_item.scssline 38 targets<p>elements and should be removed or updated after this change.Proposed markup adjustment
<span class="card__calendar-month"> - <p><%= l(meeting.start_time, format: same_month? ? "%B" : "%b") %></p> + <span><%= l(meeting.start_time, format: same_month? ? "%B" : "%b") %></span> <% unless same_month? %> - <p class="card__calendar-separator">-</p> - <p><%= l(meeting.end_time, format: "%b") %></p> + <span class="card__calendar-separator">-</span> + <span><%= l(meeting.end_time, format: "%b") %></span> <% end %> </span> <span class="card__calendar-day"> - <p><%= l(meeting.start_time, format: "%d") %></p> + <span><%= l(meeting.start_time, format: "%d") %></span> <% unless same_day? && same_month? %> - <p class="card__calendar-separator">-</p> - <p><%= l(meeting.end_time, format: "%d") %></p> + <span class="card__calendar-separator">-</span> + <span><%= l(meeting.end_time, format: "%d") %></span> <% end %> </span> <span class="card__calendar-year"> - <p><%= l(meeting.start_time, format: "%Y") %></p> + <span><%= l(meeting.start_time, format: "%Y") %></span> </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@decidim-meetings/app/cells/decidim/meetings/meeting_l/image.erb` around lines 2 - 18, Replace the <p> elements inside the calendar component in the meeting_l image cell with <span> elements to match the design system and correct HTML semantics: update the template that renders meeting.start_time and meeting.end_time (the block using same_month? and same_day? conditionals in decidim/meetings/meeting_l/image.erb) so every displayed date/month/day/year uses <span> instead of <p>, and then remove or adapt the CSS rule that targets child <p> elements (the empty:[&>p]:hidden rule in _item.scss) to target the new spans or be deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb`:
- Around line 92-94: The spec's expectation in the "it does not show separator"
example is using the wrong CSS selector; update the assertion in the example
(the block under it "does not show separator" that calls expect(subject).to
have_no_css(...)) to check for ".card__calendar-separator" instead of
".meeting__calendar-separator" so it matches the card template's class; keep the
rest of the test (subject, example name) unchanged.
---
Nitpick comments:
In `@decidim-meetings/app/cells/decidim/meetings/meeting_l/image.erb`:
- Around line 2-18: Replace the <p> elements inside the calendar component in
the meeting_l image cell with <span> elements to match the design system and
correct HTML semantics: update the template that renders meeting.start_time and
meeting.end_time (the block using same_month? and same_day? conditionals in
decidim/meetings/meeting_l/image.erb) so every displayed date/month/day/year
uses <span> instead of <p>, and then remove or adapt the CSS rule that targets
child <p> elements (the empty:[&>p]:hidden rule in _item.scss) to target the new
spans or be deleted.
In `@decidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb`:
- Around line 52-54: The spec example "it 'does not show month separator'"
currently only asserts the month text; update the example to explicitly assert
the absence of the month-separator element by adding a scoped negative matcher
against the separator element (for example: expect(subject).not_to
have_css(".card__calendar-month + .card__calendar-month-separator") or
expect(subject).not_to have_css(".card__calendar-month-separator") alongside the
existing expect(subject).to have_css(".card__calendar-month", text: "October")
so the test both confirms the month is present and that no separator node is
rendered.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
decidim-core/app/packs/stylesheets/decidim/_cards.scssdecidim-meetings/app/cells/decidim/meetings/meeting_l/image.erbdecidim-meetings/spec/cells/decidim/meetings/meeting_l_cell_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- decidim-core/app/packs/stylesheets/decidim/_cards.scss
alecslupu
approved these changes
Mar 4, 2026
This was referenced Mar 4, 2026
This was referenced Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎩 What? Why?
When a meeting has multiple dates (i.e. the end date doesn't match with the day/month of the start date), then we don't show the end date in the card view (like in the content blocks or in the index page).
This PR fixes this, and does two minor related changes:
📌 Related Issues
Testing
📷 Screenshots
Before
After
Summary by CodeRabbit
Improvements
Style
Tests