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

Feature/2704 fix aspects #3737

Merged
merged 4 commits into from Nov 25, 2012

Conversation

francesmcmullin
Copy link
Contributor

So I started out trying to improve on the changes by visualsayed in #3194 but it turns out changing things on the ruby / rails side was a total red herring, the problem was/is that none of the correct Aspect javascript was being activated, because the name of the page, at some point, changed from "AspectsIndex" to "StreamsAspects", presumably when the streams controller started taking over from other controllers.

Anyway, all that was need to fix #2704 was to rename the javascript file / page. I've also added a new cucumber test that fails without these changes, and slightly altered the behaviour of the "deselect all" link, because it annoyed me. If anyone has a better way to clear the stream, or thinks it's a bad idea, just say so =)

@francesmcmullin
Copy link
Contributor Author

no idea why Travis failed, the log just stops after "installing database_cleaner (0.9.1)", and the mysql cucumber run passed, so if someone has access to a magic rebuild button, I'd appreciate a second run =)

@DeadSuperHero
Copy link
Member

This looks good, and the build has passed. If there are no objections, I'd like to merge this. :)

@francesmcmullin
Copy link
Contributor Author

just to explain what I was talking about with the deselect all behaviour, after the first commit, clicking deselect all will deselect all, but it's confusing because neither the stream nor the link change. You can click deselect all again, and then the page refreshes and all aspects are selected. With my change, (third commit) the link toggles to say select all, and the stream is emptied. I can see the argument against emptying the stream and I'm happy to leave that out if people want, but the link should definitely toggle =)

Happy to squash the commits / rebase / make other changes if requested

@DeadSuperHero
Copy link
Member

I'm gonna test this on my localpod really quick to see how this stacks up. It'd be really nice if we could get the functionality we had back before the move over to Backbone. The specific functionality allowed users to click to add/subtract Aspects to the existing stream, rather than filtering them one at a time. I dunno if we'd ever want to have an empty stream, but I think keeping the "Select All" option is important. Maybe we could set it up so that deselecting "Select All" could fall back to the topmost Aspect to filter through?

@francesmcmullin
Copy link
Contributor Author

I totally agree this is a great feature to fix, it's something I really
missed after it stopped working and was always disappointed that google had
nothing similar (one circle at a time, why?)

To clarify, with my changes it all works again. "deselect all"only appears
when all aspects are selected, it is a convenience for when you want to go
from all aspects to a specific aspect, but it should definitely toggle
(that's how the behaviour works already) and IMO the stream should be empty
when none are selected, it will fill again as soon as something is selected.

this is a bit tricky to explain in words, if people don't have time to test
drive the changes I can post screenshots to make things clearer.
On 2012-11-20 10:56 PM, "Sean Tilley" notifications@github.com wrote:

I'm gonna test this on my localpod really quick to see how this stacks up.
It'd be really nice if we could get the functionality we had back before
the move over to Backbone. The specific functionality allowed users to
click to add/subtract Aspects to the existing stream, rather than filtering
them one at a time. I dunno if we'd ever want to have an empty stream, but
I think keeping the "Select All" option is important. Maybe we could set it
up so that deselecting "Select All" could fall back to the topmost Aspect
to filter through?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3737#issuecomment-10577711.

@jaywink
Copy link
Contributor

jaywink commented Nov 21, 2012

This would be awesome. A couple of guys at our company who I've demoed Diaspora* to, one of the first comments was "only one aspect at a time?".. The other was "so all aspects are always selected when stream is loaded?" ;)

@jhass
Copy link
Member

jhass commented Nov 25, 2012

I can haz a changelog update please? :)

@francesmcmullin
Copy link
Contributor Author

changelog update added =)

@jhass
Copy link
Member

jhass commented Nov 25, 2012

Awesome, thanks!

jhass added a commit that referenced this pull request Nov 25, 2012
@jhass jhass merged commit 9957759 into diaspora:develop Nov 25, 2012
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.

Can't select more than one aspect (for browsing)
4 participants