Skip to content

Add assemblies to participatory process#5868

Merged
Leusev merged 16 commits intodecidim:developfrom
armandfardeau:feature/add-related-assemblies-to-pp
Jun 3, 2020
Merged

Add assemblies to participatory process#5868
Leusev merged 16 commits intodecidim:developfrom
armandfardeau:feature/add-related-assemblies-to-pp

Conversation

@armandfardeau
Copy link
Copy Markdown
Contributor

@armandfardeau armandfardeau commented Mar 18, 2020

🎩 What? Why?

Add related assemblies to a participatory process

📋 Subtasks

  • Add CHANGELOG entry
  • Add tests

📷 Screenshots (optional)

image

@armandfardeau armandfardeau marked this pull request as ready for review March 18, 2020 16:34
@Leusev Leusev self-requested a review March 27, 2020 08:12
Leusev
Leusev suggested changes Mar 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Leusev Leusev left a comment

Choose a reason for hiding this comment

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

As before to start, could you check & resolve changelog file conflicts please
As short as I can, I'll continue the review
Thanks in advance

@Leusev
Copy link
Copy Markdown
Contributor

Leusev commented Mar 30, 2020

Good morning @armandfardeau ,
just realised that there isn't a linked issue to this PR:
image
As this PR is a Participatory Process's improvement, could you ask to @decidim/product if they give their approval please?
Thanks in advance

@paulinebessoles
Copy link
Copy Markdown
Contributor

paulinebessoles commented Mar 30, 2020

Hi @Leusev here is the proposal : Link assemblies to PP page.

@carolromero and @tramuntanal do you approve ?

@carolromero
Copy link
Copy Markdown
Member

carolromero commented Mar 30, 2020 •
edited
Loading

Hi, this is approved by product. For future PRs please indicate the related issue in metadecidim, so that @decidim/core can check whether it has already been approved by @decidim/product. Thanks!

@Leusev
Copy link
Copy Markdown
Contributor

Leusev commented Apr 1, 2020

Thank you @decidim/product !

By the moment, @armandfardeau could you check failing tests please? Thanks!

@armandfardeau armandfardeau requested a review from Leusev April 9, 2020 12:56
Leusev
Leusev suggested changes Apr 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Leusev Leusev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There's also a changelog file conflict, could you check please?
Thanks a lot @armandfardeau :)

...ry_processes/app/views/decidim/participatory_processes/participatory_processes/show.html.erb Outdated
</div>
<% end %>

<%= assemblies_for_participatory_process(participatory_process_assemblies) if participatory_process_assemblies.present? %>
Copy link
Copy Markdown
Contributor

@Leusev Leusev Apr 14, 2020

Choose a reason for hiding this comment

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

Would it be possible to change the way you introduce related assemblies?
I mean, instead of a partial template, to use a card_for just like it's being done above for related processes?
In this way, we avoid having hardcoded the related assembly's template and its classes, in order that if a possible redesign of the cards is carried out later, this will also be affected without any problem.

image

...patory_processes/app/helpers/decidim/participatory_processes/participatory_process_helper.rb Outdated

def assemblies_for_participatory_process(participatory_process_assemblies)
html = ""
html += %( <div class="section"> ).html_safe
Copy link
Copy Markdown
Contributor

@Leusev Leusev Apr 14, 2020

Choose a reason for hiding this comment

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

It could be possible use a card instead of having hardcoded html in this helper please?

@Leusev
Copy link
Copy Markdown
Contributor

Leusev commented Apr 28, 2020

Good morning @armandfardeau
when you have a while, could you check suggestions please?
Thanks in advance!

@Leusev Leusev self-assigned this May 11, 2020
@Leusev
Copy link
Copy Markdown
Contributor

Leusev commented May 12, 2020

Hi @armandfardeau
I don't want to be weighed, but when you can, please, check the requested changes in order to un-freeze this PR
Thanks in advance

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented May 18, 2020

Hi @Leusev, I hope to find some time this week to do this.

@Leusev Leusev self-requested a review May 26, 2020 14:55
Leusev
Leusev suggested changes May 26, 2020
View reviewed changes
@Leusev Leusev self-requested a review May 29, 2020 07:17
Leusev
Leusev suggested changes May 29, 2020
View reviewed changes
@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Jun 1, 2020

@Leusev It should be all good now :)

@Leusev Leusev self-requested a review June 3, 2020 09:35
Leusev
Leusev approved these changes Jun 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Leusev Leusev left a comment

Choose a reason for hiding this comment

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

All perfect now, thanks a lot for your job @armandfardeau 👍

@Leusev Leusev merged commit 60f75ac into decidim:develop Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Leusev Leusev Leusev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Leusev Leusev

Labels

in-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@armandfardeau @Leusev @paulinebessoles @carolromero @Quentinchampenois @tramuntanal