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

Consultations: unify navigation #3524

Merged
merged 9 commits into from
May 30, 2018
Merged

Conversation

mrcasals
Copy link
Contributor

@mrcasals mrcasals commented May 28, 2018

🎩 What? Why?

This PR overhauls the navigation for decidim-consultations, as I find it very hard to understand, by doing this:

  • The topbar link to Consultations used to take to the active consultation, if only 1. Now it always takes to the index page, as you might want to see the past or upcoming consultations. This is consistent to the other navigations.
  • Uses the consultation card in the index page
  • In the consultation page, there were two lists: "Highlighted consultations" and "Other consultations". The first one is now called "Highlighted questions", since that's what it's listing. The last one is now called "Questions for this consultations" and lists all questions for the current consultation. This is to avoid a weird title when no highlighted questions were found for the given consultation, and we had the "Other consultations" text, which was very weird to me.
  • Removes the "finished consultations" view. It was only listing finished and active ones, but was missing the upcoming ones. This is now superseded by the filtered index page.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry

📷 Screenshots (optional)

Index page:
Description

Show page:

@mrcasals
Copy link
Contributor Author

@decidim/product thoughts on this?

@mrcasals mrcasals force-pushed the consultstions/unify-navigation branch from 565254a to a3bf85a Compare May 28, 2018 12:55
@mrcasals mrcasals force-pushed the consultstions/unify-navigation branch from a3bf85a to a168e58 Compare May 28, 2018 13:34
@mrcasals mrcasals force-pushed the consultstions/unify-navigation branch from a168e58 to d9e0ce7 Compare May 28, 2018 13:45
oriolgual
oriolgual previously approved these changes May 28, 2018
@andreslucena
Copy link
Member

The topbar link to Consultations used to take to the active consultation, if only 1. Now it always takes to the index page, as you might want to see the past or upcoming consultations. This is consistent to the other navigations.

I'm not so sure about this one. I'd prefer to have this kind of logic, but also for other kind of contents (for instance, when there's only a participatory process or only an assembly). This kind of use case (Decidim with Only 1 Participatory Process) is pretty common, and to display /processes when there is only one it's unnecessary for me.

But I'd love to hear thoughts from the rest of @decidim/product

@mrcasals
Copy link
Contributor Author

@andreslucena I'm not really for this kind of navigations, to me a link should always take you to the same place consistently, and if I click a link called "Consultations" (in plural), I expect it to take me to the index page, but I see your point. The problem with the previous navigation for consultations (the one in master) is that, in the case when there's only one active consultation, but many finished and upcoming, there was no way to see the upcoming ones. The topbar link takes you to the active page, and from there you could visit a page where the finished and active ones were listed. But there was no way to see the upcoming ones. This was a problem to my while trying to debug #3457

@josepjaume
Copy link
Contributor

For consistency reasons, I think we should merge this in. If we are to rethink the way navigation works when dealing with collections, we should discuss it with @decidim/web-design and unify it.

@mrcasals
Copy link
Contributor Author

Build is green, conflicts were only on the CHANGELOG file and this PR was approved.

I'm merging this for consistency reasons. We can open a new issue discussing any change on navigation patterns for the platform if anyone thinks it's needed.

@mrcasals mrcasals merged commit bf592a0 into master May 30, 2018
@ghost ghost removed the status: WIP label May 30, 2018
@mrcasals mrcasals deleted the consultstions/unify-navigation branch May 30, 2018 12:43
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.

4 participants