Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scheduled debate date to awaiting debate petitions view #456

Conversation

leandroalemao
Copy link
Contributor

Hi people,

I've added the debate schedule date to the awaiting debate petitions view. Useful to check the debate date before clicking on the petition.

image

@@ -44,4 +44,13 @@ def api_date_format(date_time)
end
end
end

def to_be_debated_on_in_words(date)
return unless date.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing the checking for the debate date here you end up with and empty <p> tag which is undesirable - we should use scheduled_debate_date? on the petition in the view and remove this check. E.g:

<% if petition.scheduled_debate_date? %>
<p><%= to_be_debated_on_in_words(petition.scheduled_debate_date) %></p>
<% end %>

@pixeltrix
Copy link
Contributor

Other than the comments it looks 👍

@henryhadlow do we want to do relative dates, eg. 'To be debated today', 'To be debated tomorrow' ?

@leandroalemao
Copy link
Contributor Author

@pixeltrix tks for replying..

I agree.. and I'm making the changes right now and waiting for @henryhadlow reply.

👍

@@ -45,11 +46,12 @@ def api_date_format(date_time)
end
end

def to_be_debated_on_in_words(date)
def to_be_debated_on_in_words(date, today = Date.today)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be Date.current to take account of application timezone

@@ -44,4 +45,14 @@ def api_date_format(date_time)
end
end
end

def to_be_debated_on_in_words(date, today = Date.current)
return unless date.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this check here now since we're checking in the view for the presence of the date

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree.. sorry, I forgot to remove it.. but it's all done now.. commit 4700c6f 👍 😄 tks

@pixeltrix pixeltrix merged commit 077ced0 into alphagov:master Apr 22, 2016
@pixeltrix
Copy link
Contributor

@leandroalemao thanks!

@leandroalemao
Copy link
Contributor Author

@pixeltrix, I'm glad to see you've merged my pull request.. 👍 😄 I'm willing to contribute more and more. Tks!

@pixeltrix
Copy link
Contributor

@leandroalemao it was a pleasure - if you're looking for something else to work on then #450 is a nice and easy one but if you're looking for something a bit deeper then #458 should be interesting. Feel free to ask questions if you need anything clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants