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

Added council suggestions for comparison tool #698

Merged
merged 3 commits into from
Dec 10, 2024
Merged

Conversation

lucascumsille
Copy link
Contributor

@lucascumsille lucascumsille commented Oct 14, 2024

Fixes: #686

I left some comments regarding things that are pending, but front-end related I think it should be ready.

  • We need to pull a list of 5 councils based on overall similarity like we do in CAPE. I picked 5 randomly, it seemed like a sweet spot between giving the user enough options, but not overwhelm them with too many choices.
  • Replace the placeholder councils with a loop based on overall similarity.
  • We need to add commas to the list of councils, but I can do that using CSS, however I thought we could keep doing it with Django like we have done in other part of the website. Let me know which approach you prefer.

Preview here:

Screenshot 2024-10-21 at 14 19 38 Screenshot 2024-10-21 at 14 19 12

@lucascumsille
Copy link
Contributor Author

@zarino and @struan I think I'm done with this PR, but we need the back-end logic to be implemented. I left some comments at the beginning.

@zarino
Copy link
Member

zarino commented Nov 6, 2024

@lucascumsille I’ve wired this up, so that a maximum of 5 overall related councils are passed to the template, removing any that are already selected for comparison. I simplified some of the wording, and used flex layout for the links, rather than concatenating them into a sentence with commas.

Screenshot 2024-11-06 at 16 03 28

@zarino zarino requested a review from struan November 6, 2024 16:30
@zarino zarino marked this pull request as ready for review November 6, 2024 16:30
@zarino
Copy link
Member

zarino commented Nov 6, 2024

Marking as ready for review by @struan, so he can check I my Python isn’t too horrendous, and work out whether we should fix the linting errors or not.

@zarino zarino changed the title WIP Added council suggestions for comparison tool Added council suggestions for comparison tool Nov 6, 2024
@struan struan self-assigned this Nov 19, 2024
@struan struan force-pushed the 686-nearest-neighbour branch from 2253446 to e68ce6b Compare November 20, 2024 11:30
Copy link
Member

@struan struan left a comment

Choose a reason for hiding this comment

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

this looks good.

I've pushed some commits to update the black version and fix the resulting linting issues. We were using an old version so I've updated it to the latest version.

scoring/views.py Outdated Show resolved Hide resolved
context["comparisons"] = comparisons

for group in council.get_related_councils(5):
if group["type"].slug == "composite":
Copy link
Member

Choose a reason for hiding this comment

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

why does this only apply if it's composite?

Copy link
Member

Choose a reason for hiding this comment

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

We only want the 5 "overall" similar councils, not the other types of similarity.

Copy link
Member

Choose a reason for hiding this comment

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

(I just copied this block, wholesale, from the caps app)

@struan struan force-pushed the 686-nearest-neighbour branch from 3bdc2a9 to b8bf1cb Compare November 20, 2024 11:45
@zarino zarino assigned zarino and struan and unassigned struan and zarino Dec 5, 2024
@struan struan force-pushed the 686-nearest-neighbour branch from b8bf1cb to ada8938 Compare December 10, 2024 14:16
@struan struan force-pushed the 686-nearest-neighbour branch from ada8938 to 8c2fe10 Compare December 10, 2024 14:26
@struan struan merged commit 8c2fe10 into master Dec 10, 2024
5 checks passed
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.

[scorecards] look at including the nearest neighbour tool on council page
3 participants