Fix multiple days in debates #33

IvanOrtizp · 2020-11-13T09:42:00Z

🎩 What? Why?

It is used to add the end date of a debate, and appear on the view in addition to the start date with the day and month.

📌 Related Issues

Link your PR to an issue

Related to #?
Fixes Debates with multiple days #30

Testing

Testing to verify that the dates appear correctly

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

📷 Screenshots

♥️ Thank you!

microstudi

primer exemple de review (les reals son en anglès):

Falta millorar la descripció del que has fet, no cal posar "I fix", es millor posar de manera mes neutre que fa el pr que ho pugui entendre una persona que no sap de que va.
Si el canvi es visual, cal incloure un screenshot del abans i després (podeu enganxar una imatge a la descripció del github, es puja automticament)
Quan feu commits, es important que la descripció sigui una mica mes entenedora (no posar "a" com a descripció). Sempre de la forma imperativa, com contestant a la pregunta: "If i apply this change it will ...", els 3 punts seria el que hauries d'escriure.
Un cop posat el PR cal mirar si passa els testos, el primer de tots es el d'estil "Lint", si hi ha errors cal arreglar-ho. Hi ha eines automatiques per fer-ho depenent del tipus d'error:

bundle exec rubocop -a
bundle exec erblint decidim-*/app/**/*.erb --autocorrect

IvanOrtizp · 2020-11-26T10:43:00Z

Perfecte, ara faig els canvis, Gràcies. El jue, 26 de nov. de 2020 a la(s) 11:38, Ivan Vergés ( [email protected]) escribió:

…

***@***.**** requested changes on this pull request. primer exemple de review (les reals son en anglès): - Falta millorar la descripció del que has fet, no cal posar "I fix", es millor posar de manera mes neutre que fa el pr que ho pugui entendre una persona que no sap de que va. - Si el canvi es visual, cal incloure un screenshot del abans i després (podeu enganxar una imatge a la descripció del github, es puja automticament) - Quan feu commits, es important que la descripció sigui una mica mes entenedora (no posar "a" com a descripció). Sempre de la forma imperativa, com contestant a la pregunta: "If i apply this change it will ...", els 3 punts seria el que hauries d'escriure. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#33 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARJEF4YGJB2QONC6FQI35Y3SRYVZHANCNFSM4TULVB2A> .

davidbeig · 2020-12-15T22:42:49Z

decidim-proposals/spec/shared/proposal_form_examples.rb

@@ -139,7 +139,6 @@
    context "when the has address checkbox is checked" do
      let(:has_address) { true }

-      # rubocop:disable RSpec/EmptyExampleGroup


This comments should not be deleted. Since this PR will be about decidim-debates, we should not change things in other components.

davidbeig · 2020-12-15T22:43:01Z

decidim-proposals/spec/shared/proposal_form_examples.rb

@@ -152,7 +151,6 @@
          it { is_expected.to be_invalid }
        end
      end
-      # rubocop:enable RSpec/EmptyExampleGroup


The same for this one

davidbeig · 2020-12-15T22:47:41Z

decidim-core/app/assets/stylesheets/decidim/layouts/_view.scss

@@ -12,6 +12,11 @@
  }
 }

+.icon-arrow-thin-right {


Do we want this class to be used in the whole of decidim or will it only be used in the cards used by debates? If so maybe it should go in another scss file related to cards or to debates.

verarojman

Good work! I left some suggestions to improve the code and ensure it follows some standards that help us developers better understand each other 🤖 🌱

Another thing that helps is to make sure your commit messages are descriptive (there are loooooots of guides of how to do it, I picked one randomly for you: https://chris.beams.io/posts/git-commit/#seven-rules)

Quick tip: If you haven't pushed changes yet, you can amend commits

And, one question: Why did you make so many commits? :D

verarojman · 2020-12-15T22:54:56Z

decidim-debates/app/views/decidim/debates/debates/show.html.erb

@@ -76,14 +90,12 @@ edit_link(
            <% end %>
          </span>
        </div>
-


It is unnecessary to remove these blank lines, and it makes it more difficult to focus on the real changes this PR is making ;) The automatic erb linter should take care that .erb files have a concise style, you don't need to worry about it!

verarojman · 2020-12-15T22:55:31Z

decidim-debates/app/views/decidim/debates/debates/show.html.erb


-        <% if endorsements_enabled? && allowed_to?(:endorse, :debate, debate: debate) %>
+          <% if endorsements_enabled? && allowed_to?(:endorse, :debate, debate: debate) %>


Woops! Wrong indentation 🙆🏽‍♀️

verarojman · 2020-12-15T23:02:06Z

decidim-debates/app/views/decidim/debates/debates/show.html.erb

+        <div>
+          <%= l debate.start_time, format: "%d" %> <span class="extra__month"> <%= l debate.start_time, format: "%B" %></span>
+          <div class="extra__time text-small"> <%= l debate.start_time, format: "%H:%M" %> - <%= l debate.end_time, format: "%H:%M" %></span></div>
+        </div>
+            <% else %>
+        <div>
+          <%= l debate.start_time, format: "%d" %> <span class="extra__month"> <%= l debate.start_time, format: "%B" %></span>
+          <div class="extra__time text-small"> <%= l debate.start_time, format: "%H:%M" %></span></div>
+        </div>
+        <%= icon "arrow-thin-right", class: "icon-arrow-thin-right" %>
+        <div>
+          <%= l debate.end_time, format: "%d" %> <span class="extra__month"> <%= l debate.end_time, format: "%B" %></span>
+          <div class="extra__time text-small"> <%= l debate.end_time, format: "%H:%M" %></span></div>
+        </div>
+            <% end %>
+    </div>
+            <% end %>


Nice! It's working as it should :) To improve it, I would:

Run erblint to correct the indentation (check the decidim docs if you're not sure how to do it)

Extracting the logic into a Rails helper would be super super nice 👌🏽 That way this file would be more readable and therefore easy to understand.

verarojman · 2020-12-15T23:04:02Z

decidim-debates/spec/system/multiple_date_spec.rb

+    it "shows the start date" do
+      within "extra__date" do
+        expect(page).to have_content(debate.start_time.strftime("%d"))
+        expect(page).to have_content(debate.start_time.strftime("%H:%M"))
+      end
+    end
+
+    it "shows the end date" do
+      within "extra__date" do
+        expect(page).to have_content(debate.end_time.strftime("%d"))
+        expect(page).to have_content(debate.end_time.strftime("%H:%M"))
+      end
+    end


Good! You should add the other cases too (when only one day is displayed, when there is no end date...)

IvanOrtizp · 2020-12-17T09:02:07Z

Thanks for the review

IvanOrtizp added 6 commits November 13, 2020 15:00

change

83d3f40

a

cb2aacb

a

1076e39

guardar cambis

a1c5752

a

159dc1d

Fix multiple days in debates

f18dab0

IvanOrtizp changed the title ~~change~~ Fix branch Nov 20, 2020

IvanOrtizp added 3 commits November 20, 2020 17:49

Fix multiple days in debates

7103bd1

Fix multiple days in debates

a492491

Fix multiple days debates

afdbda6

IvanOrtizp changed the title ~~Fix branch~~ Fix multiple days in debates Nov 26, 2020

microstudi suggested changes Nov 26, 2020

View reviewed changes

IvanOrtizp added 6 commits November 26, 2020 16:22

Fix multiple days debates

a8a13a7

If i apply this change, the end date of debate will ve added

9d2096e

The end date of debate will be added

0da9980

The end date of debate will be added

2926662

correct the lint errors

95a6bdd

Fix lint errors

d442342

IvanOrtizp marked this pull request as draft December 3, 2020 10:28

IvanOrtizp added 3 commits December 3, 2020 17:15

fix rspec errors

8518855

fix rspec errors

8199975

fix rspec errors

43bc3f9

IvanOrtizp force-pushed the fix-branch branch from 3a7b83e to 43bc3f9 Compare December 9, 2020 10:40

verarojman added the practiques Pràctiques Decidim label Dec 9, 2020

IvanOrtizp added 5 commits December 11, 2020 17:46

add first test for multiple dates on debates

09503cb

add test for multiple dates on debates

d5a303a

add rspec test

0471aa8

add rspec test for multiple dates

0ca84ac

add rspec test for multiple dates

29294c6

IvanOrtizp added 3 commits December 15, 2020 14:57

add rspec test for multiple dates

5d15a74

add rspec test for multiple dates

6a80f5a

add rspec test for multiple dates

9feace2

davidbeig reviewed Dec 15, 2020

View reviewed changes

verarojman suggested changes Dec 15, 2020

View reviewed changes

Update multiple_date_spec.rb

107a7d5

IvanOrtizp added 10 commits December 17, 2020 17:04

correct the arrow style

1155564

add the rspec test for multiple dates

b0713ac

add the rspec test for multiple dates

a9b93cd

add the rspec test for multiple dates

8a2d06e

add the rspec test for multiple dates

e63357f

add the rspec test for multiple dates

f3bf3ad

add the rspec test for multiple dates

f7c2ff9

add the rspec test for multiple dates

74a44e5

correct the rspec errors

7594648

correct the rspec errors

b34c4f7

IvanOrtizp marked this pull request as ready for review March 11, 2021 11:44

microstudi closed this Jul 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiple days in debates #33

Fix multiple days in debates #33

IvanOrtizp commented Nov 13, 2020 •

edited

Loading

microstudi left a comment •

edited

Loading

IvanOrtizp commented Nov 26, 2020 via email

davidbeig Dec 15, 2020

davidbeig Dec 15, 2020

davidbeig Dec 15, 2020

verarojman left a comment •

edited

Loading

verarojman Dec 15, 2020

verarojman Dec 15, 2020

verarojman Dec 15, 2020

verarojman Dec 15, 2020

IvanOrtizp commented Dec 17, 2020


		<% if endorsements_enabled? && allowed_to?(:endorse, :debate, debate: debate) %>
		<% if endorsements_enabled? && allowed_to?(:endorse, :debate, debate: debate) %>

Fix multiple days in debates #33

Fix multiple days in debates #33

Conversation

IvanOrtizp commented Nov 13, 2020 • edited Loading

🎩 What? Why?

📌 Related Issues

Testing

📋 Checklist

📷 Screenshots

microstudi left a comment • edited Loading

Choose a reason for hiding this comment

IvanOrtizp commented Nov 26, 2020 via email

davidbeig Dec 15, 2020

Choose a reason for hiding this comment

davidbeig Dec 15, 2020

Choose a reason for hiding this comment

davidbeig Dec 15, 2020

Choose a reason for hiding this comment

verarojman left a comment • edited Loading

Choose a reason for hiding this comment

verarojman Dec 15, 2020

Choose a reason for hiding this comment

verarojman Dec 15, 2020

Choose a reason for hiding this comment

verarojman Dec 15, 2020

Choose a reason for hiding this comment

verarojman Dec 15, 2020

Choose a reason for hiding this comment

IvanOrtizp commented Dec 17, 2020

IvanOrtizp commented Nov 13, 2020 •

edited

Loading

microstudi left a comment •

edited

Loading

verarojman left a comment •

edited

Loading