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

Maps optional in meetings #7954

Merged
merged 5 commits into from
Jun 7, 2021
Merged

Maps optional in meetings #7954

merged 5 commits into from
Jun 7, 2021

Conversation

entantoencuanto
Copy link
Contributor

@entantoencuanto entantoencuanto commented May 6, 2021

🎩 What? Why?

This PR:

  • Adds a setting to meetings component to disable maps for meetings. This option is true by default
  • Changes meeting show in front to hide map even if the meeting is online or hybrid when maps are disabled for component
  • Hides map on meetings index components if disabled for them
  • Removes map column in admin panel when maps are disabled in the component
  • Adds some system tests

📌 Related Issues

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@entantoencuanto entantoencuanto marked this pull request as ready for review May 11, 2021 21:00
@@ -1,6 +1,8 @@
<%= render partial: "decidim/shared/component_announcement" %>

<%= cell "decidim/meetings/meetings_map", search.results %>
<% if current_component.settings.maps_enabled? %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea but could we also hide the map when there are no meetings with map pointers associated to them (online meetings only)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a good idea, if @decidim/product approves it I'll include the change

@carolromero
Copy link
Member

I think it's a good idea, if @decidim/product approves it I'll include the change.

Yes, we agree! Let's include it then, thanks!

@leio10 leio10 added module: meetings type: feature PRs or issues that implement a new feature labels Jun 7, 2021
Copy link
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

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

Good job! And it was very easy to review, thanks! 🤩

@@ -95,6 +95,7 @@ class Meeting < Meetings::ApplicationRecord

TYPE_OF_MEETING.each do |type|
scope type.to_sym, -> { where(type_of_meeting: type.to_sym) }
scope "not_#{type}".to_sym, -> { where.not(type_of_meeting: type.to_sym) }
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 I think this is ok and I'd not change it now, but I feel we are rewriting enums here. 😕

@leio10 leio10 merged commit ec59882 into develop Jun 7, 2021
@leio10 leio10 deleted the 7286-maps_optional_in_meetings branch June 7, 2021 10:04
@leio10 leio10 mentioned this pull request Jun 7, 2021
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review module: meetings type: feature PRs or issues that implement a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Meetings map is optional in component
4 participants