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 custom steps CTA for Process cards index #6284

Merged
merged 6 commits into from Jul 29, 2020
Merged

Add custom steps CTA for Process cards index #6284

merged 6 commits into from Jul 29, 2020

Conversation

armandfardeau
Copy link
Contributor

@armandfardeau armandfardeau commented Jul 8, 2020

🎩 What? Why?

📌 Related Issues

📋 Subtasks

  • Add tests

📷 Screenshots (optional)

Capture d’écran 2020-07-08 à 11 24 36

@armandfardeau armandfardeau marked this pull request as ready for review July 8, 2020 09:25
@virgile-dev
Copy link
Contributor

Hey @carolromero
This is ready to be reviewed by product.
You can test it here https://aea.cabana.osp.cat/processes
See that the process card CTA says let's go instead of "Take part".
Capture d’écran 2020-07-08 à 18 24 52
It's configured here : https://aea.cabana.osp.cat/admin/participatory_processes/elearning/steps/89/edit
I've sent you an invite to get admin access to this process on you localret email adress.

@carolromero
Copy link
Member

Hey @virgile-dev I've checked my mail and I have an invitation for another instance, this one: https://demo.decidim.opensourcepolitics.eu

Will you send me the invitation to the other instance? 😊

@Leusev Leusev self-requested a review July 14, 2020 08:33
@Leusev Leusev self-assigned this Jul 14, 2020
Copy link
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.

Hello @armandfardeau
it seems that it may be some unused translations, could take a look please?
image

Thanks in advance!

Copy link
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.

Ok, thanks @armandfardeau
It only remains to wait for @Product 's testing confirmation.
Thanks to all !

@andreslucena
Copy link
Member

Hi guys. From @decidim/product perspective, we thought on doing this also on Proposals, but our final decision was that this would get bad fast, as there are just too many things that you'd want to customize on Decidim.

So, the consensus after discussing this also with @decidim/core team at the moment was to use an awesome module that made @ahukkanen, the decidim-module_term_customizer. The idea was also to add it as a default commented module so other installations can easily find about it, and on the future when we have resources for having an in-house development team, that we could also support and "official" version for this module, although this is like a wishlist at the moment. As for now, we were using for the last months in Barcelona as it helped us a lot in our organization's instances and processes. See #5581.

We're closing this as a "won't fix", thanks for the PR tough.

@virgile-dev
Copy link
Contributor

virgile-dev commented Jul 16, 2020

@andreslucena Thanks for providing an alternative way of doing this through the term customizer.
I'll test as soon as I get a chance.

Just to make sure we all talking about the same thing, this PR relates to this Meta Decidim proposal that was made by the city of Barcelona and got accepted. I'm bit surprised to see this was rejected in the last instance of validation (after the PR was made and validated by the maintainers). Moreover we are adding new fields a reusing the custom step CTA, so it's not like we are not adding configuration or anything.

@carolromero do we want to change the state of that proposal ?

@ahukkanen
Copy link
Contributor

I must also add that Term Customizer does not allow to change the CTA path, so the change suggested by this PR would be very welcomed in my opinion. As seen in this discussion, it's been already requested by various parties, including some of the towns we are working with.

It's better not to block admin users from doing what they want to do until there is a better technical way to do it.

Regarding the "better technical way", I want to add that the content management side of Decidim is something that constantly gets criticism among the admin users. This is also why Decidim is full of various admin panel settings currently, sort of like managing an airplane dashboard. Many users feel like "if you don't have 2 years of formal training on how to use it, you better stay far away from it".

The content management side is an issue of its own and it's not as easy to tackle as this issue by itself but it is the reason why this type of settings need to be added in the first place. In my opinion, Decidim should not dictate what how the pages are built and what functionality they contain, e.g. in terms of the process listing page. They should be defined by an expert admin user ("site builder") who would use the content blocks concept in Decidim to build them e.g. by adding a "process list" block where you'd also define the CTA path. This would decouple settings like this from the base objects (in this case processes) and would allow the site builder to define the CTA paths as they wish through the CMS.

I understand building something like that would take a lot of effort and I'm sure there are more important issues in the roadmap right now. Meanwhile, I would warmly welcome this change until the CMS side is re-thought.

As @virgile-dev, I would be also very surprised to see that an accepted feature does not get into the core.

@andreslucena
Copy link
Member

this PR relates to this Meta Decidim proposal that was made by the city of Barcelona and got accepted
As @virgile-dev, I would be also very surprised to see that an accepted feature does not get into the core.

And I'm surprised that a Meta link wasn't added on the template on the first place 😅 - please let's try to keep this traceability so we don't have cases of "too many cooks" in @decidim/product as the development process is pretty slow (we're talking about a proposal made 9 months ago, retaken by @virgile-dev 3 months ago, with a change of mind on how to handle these customizations of texts by @decidim/product on #5881 that was made 7 months ago, etc)

I actually didn't take in account that this apart from changing the CTA text it also changes the CTA link/path, sorry about that, I'm reopening this, although I'm not so sure about having two ways for changing these kinds of texts... As we already have these kind of customization on other CTAs (like the one from the home), I think we can leave it like it's proposed here.

The content management side is an issue of its own and it's not as easy to tackle as this issue by itself but it is the reason why this type of settings need to be added in the first place.

I agree. We discussed in the past that we should have like a simpler/easier way for managing the admin and then an "advanced settings" were you have access to all the current complexity.

@andreslucena andreslucena reopened this Jul 17, 2020
@stale stale bot removed the wontfix label Jul 17, 2020
@virgile-dev
Copy link
Contributor

Thanks @andreslucena, sorry for not putting the metadecidim link, our bad with @armandfardeau

@Leusev I think you can now go ahead and merge it :)

Thanks in advance :)

@Leusev Leusev self-requested a review July 22, 2020 09:24
Copy link
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.

Hello @armandfardeau
meanwhile, could you kindly resolve the i18-tasks.yml conflict please?
image

Thanks in advance!

@tramuntanal
Copy link
Contributor

@Leusev , @armandfardeau I've resolved the conflict via GitHub interface. Let's see now if tests succeed

@armandfardeau
Copy link
Contributor Author

@tramuntanal Thank you


def step_cta_path
if model.active_step&.cta_path.present?
resource_path + "/" + model.active_step.cta_path
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @armandfardeau
making some tests, I've seen that if there's an url param, like locale, due to having changed language by dropdown, the resulting step_cta_path looks a bit weird, as follows:

  • /processes/blanditiis-labore?locale=ca/test-path-url
    image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! I'm trying to reproduce the problem you showed, but I can't do it, do you have an idea? Here's what I've tried to do:

context "when user switch locale" do
  before do
    visit decidim_participatory_processes.participatory_processes_path
     within_language_menu do
       click_link "Català"
     end
  end

  it "displays the regular cta button" do
    within "#participatory_process_#{participatory_process.id}" do
      expect(page).to have_link("Take action!", href: "/processes/#{participatory_process.slug}/my_path")
    end
  end
end

This currently should not work, but I couldn't manage to display the local param

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @armandfardeau
The steps to reproduce would be:

  1. Change language by dropdown. This adds language locale to url
  2. Click on processes menu option
  3. Click on process CTA button
  4. The url has changed with locale param between "normal" url and custom url

cta_record_locale

def participatory_process_cta_path(process)
return participatory_process_path(process) if process.active_step&.cta_path.blank?

participatory_process_path(process) + "/" + process.active_step.cta_path
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here I guess..

@Leusev
Copy link
Contributor

Leusev commented Jul 27, 2020

Furthermore last commented, could you also check conflicting files please?
Thanks in advance!

@armandfardeau
Copy link
Contributor Author

Furthermore last commented, could you also check conflicting files please?
Thanks in advance!

Thanks @Leusev ! Should be good now

@Leusev Leusev self-requested a review July 29, 2020 07:18
Copy link
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.

Ok, all ok now @armandfardeau 😄
Thanks a lot for your work! 👍

@Leusev Leusev merged commit 627da46 into decidim:develop Jul 29, 2020
@armandfardeau armandfardeau deleted the feature/participatory-processes-index-custom-cta branch July 29, 2020 14:21
Quentinchampenois pushed a commit to Quentinchampenois/decidim that referenced this pull request Aug 14, 2020
* Add custom steps CTA for Process cards index

* Ignore unused

* Fix query param interpolation

* Fix ignore unused keys

Co-authored-by: Oliver Valls <oliver.vh@coditramuntana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants