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 Voted? column to admin/events view #705

Merged
merged 6 commits into from Oct 14, 2015

Conversation

altsalt
Copy link

@altsalt altsalt commented Aug 7, 2015

Background and textual notification to easily see which proposals a user has or has not voted upon.

Requesting merge.

@@ -34,6 +34,8 @@
%b Difficulty
%th
%b State
%th
%b Voted?
Copy link
Member

Choose a reason for hiding this comment

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

Not very clear what this means. Voted on at all? Voted on by you?

@hennevogel
Copy link
Member

Otherwise, nice. Thanks! 👍

@altsalt
Copy link
Author

altsalt commented Aug 7, 2015

I was trying to avoid using too much horizontal space and the text of "Yes" and "Not yet" seem self-explanatory to me.

Thoughts on re-wording?

@hennevogel
Copy link
Member

Well if you're trying to save horizontal space you should have put this into the 'rating' column with

- if event.voted?(event, current_user)
  Your Vote: number
- else
  %span{style = "background-color: #{bgcolor}" }
    =link_to "Vote" event_path(event)

@altsalt
Copy link
Author

altsalt commented Aug 11, 2015

I re-did the patch as suggested, using numbers not stars.
Dropped the background color as differences in text are noticeable.

p.s. I have also patched in pull #704

@@ -34,6 +34,8 @@
%b Difficulty
%th
%b State
%th
Copy link
Contributor

Choose a reason for hiding this comment

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

The new column needs to go too with the current implementation

@differentreality
Copy link
Contributor

I agree with Henne about not horizontally expanding.
However a visual indication of not-voted proposals could be useful. Perhals a success/warning label (http://getbootstrap.com/components/#labels)?

@altsalt
Copy link
Author

altsalt commented Aug 11, 2015

Good catch on the extra column!

I wrapped both notifications in a label, thoughts?

@differentreality
Copy link
Contributor

screenshot-15

@differentreality
Copy link
Contributor

I like the label indication, @hennevogel you?

I would change the wording though to "Your vote:" and "No vote" (because 'You haven't voted' is too long)

screenshot-19

@altsalt
Copy link
Author

altsalt commented Aug 15, 2015

"Haven't voted" is 13 characters
"Not voted" is 9 characters
"Not rated" is 9 characters
"Your vote: #" is 12 charaters

I don't think the word length matters too much as long as it is is clear.
Personally, I think "Your vote: #" and "Not rated" read best.

@differentreality
Copy link
Contributor

"Not rated" sounds to me as if the event has not rating at all.
"No vote" would use the same word origin as "Your vote" and I think it is more likely to be less confusing about the fact that it corresponds to this particular user's vote. Does this only make sense to me?

@hennevogel
Copy link
Member

We're not in the character saving business. We are in the guiding-users-to-do-useful-things business :-) So if you think about what this label should convey it's something like:

You haven't voted yet, please click here immediately to do so!

Now you can abbreviate this in any way as long as it does not loose it's meaning. My personal preference would be: 'Vote now'

@differentreality
Copy link
Contributor

@hennevogel this PR contains changes which we have already merged (#704). Github says we can go ahead and merge this, but I'd like to re-confirm that with you. Should we merge or have the PR rebased?

@hennevogel
Copy link
Member

If you're not sure click the 'command line instructions' link below and see what happens if you pull it in manually :-)

@differentreality
Copy link
Contributor

Events with 0 average rating have not been rated by the current_user either.

Should we move the code for the label after the else code?

@hennevogel
Copy link
Member

What is this PR waiting for?

@differentreality
Copy link
Contributor

I ll merge it and do a grooming fix later for the label to appear when no one has voted.

differentreality added a commit that referenced this pull request Oct 14, 2015
added Voted? column to admin/events view
@differentreality differentreality merged commit 2e692f7 into openSUSE:master Oct 14, 2015
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