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

ZA - remove place filter on MP profiles list page #2237

Merged
merged 1 commit into from Feb 7, 2017

Conversation

struan
Copy link
Member

@struan struan commented Feb 3, 2017

This removes the dropdown for only showing MPs by area as it's largely been made redundant by the improved search.

Fixes #2233

@coveralls
Copy link

Coverage Status

Coverage remained the same at 53.993% when pulling b76ea8d on 2233-za-remove-search-dropdown into 77f8ad6 on master.

@struan struan requested a review from mhl February 3, 2017 11:25
Copy link
Contributor

@mhl mhl left a comment

Choose a reason for hiding this comment

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

The code change looks good, but I'm afraid I'm going to be a bit picky about the commit message. (Which I guess might not come as a crashing surprise ;))

Could you include in the commit message a brief explanation of why this is being done? Just linking to the issue (particularly just with an issue number, not a full URL) is not ideal - there's some justification for that here: http://mysociety.github.io/coding-standards.html#commit-messages

Also, I think it'd be worth adding a comment about how this removes the place filter from all position view pages, not just the MP profiles page? (To be clear, I'm OK with that - my feeling is that it's not very helpful on any of the pages - but I think it's worth a comment, since the commit subject currently suggests this a more specific change.)

@mhl
Copy link
Contributor

mhl commented Feb 4, 2017

(BTW, when I said it wasn't likely to be a surprise that I'm commenting on commit messages, that was alluding to the fact that I'm generally picky about them, nothing particular to you!)

@struan struan force-pushed the 2233-za-remove-search-dropdown branch from b76ea8d to e6fbcc2 Compare February 6, 2017 15:37
@coveralls
Copy link

Coverage Status

Coverage remained the same at 53.993% when pulling e6fbcc2 on 2233-za-remove-search-dropdown into a24d60f on master.

@mhl
Copy link
Contributor

mhl commented Feb 6, 2017

👍 Great, thanks!

We're removing the place filter from search pages partly because the
improved search makes it redundant and also because people mostly don't
know which area to use so it's not actually very useful.

Fixes #2233
@struan struan force-pushed the 2233-za-remove-search-dropdown branch from e6fbcc2 to b7bbeee Compare February 7, 2017 11:23
@struan struan merged commit b7bbeee into master Feb 7, 2017
@mysociety-pusher mysociety-pusher deleted the 2233-za-remove-search-dropdown branch June 7, 2019 14:20
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.

None yet

3 participants