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 message and disabled the dropdown in email view when no participants are present #65

Merged
merged 4 commits into from Jul 11, 2017

Conversation

Projects
None yet
4 participants
@nikhilgupta1211
Contributor

nikhilgupta1211 commented Jun 29, 2017

No description provided.

if(users.length == 0 ) {
$('#emailbody').prepend('#{error_message}');
}
}

This comment has been minimized.

@bgeuken

bgeuken Jun 29, 2017

Member

I know, there was already inline js. But could you move that to the javascript assets? I would prefer to not have inline javascript.

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2017

Coverage Status

Coverage decreased (-0.05%) to 91.95% when pulling 38d2b09 on nikhilgupta1211:selectinemail into a74359b on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2017

Coverage Status

Coverage remained the same at 92.003% when pulling 27d8954 on nikhilgupta1211:selectinemail into a74359b on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 30, 2017

Coverage Status

Coverage remained the same at 92.003% when pulling e4f99c8 on nikhilgupta1211:selectinemail into a74359b on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 30, 2017

Coverage Status

Coverage remained the same at 92.003% when pulling ccbb10c on nikhilgupta1211:selectinemail into a74359b on openSUSE:master.

%a{:onclick => "user_email(#{users_for_event('canceled')})"} Cancel
- %w[All Accepted Incomplete Submitted Approved Cancel].each do |s|
%li
%a{id: 'state' + s, "data-users" => users_for_event(s.downcase) } #{s}

This comment has been minimized.

@bgeuken

bgeuken Jun 30, 2017

Member

Nice that you cleaned that up 👍

@ChrisBr

This comment has been minimized.

Member

ChrisBr commented Jun 30, 2017

Please add some screenshots :)

@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Jun 30, 2017

image
event_emails#new when no participants are present

@ChrisBr

This comment has been minimized.

Member

ChrisBr commented Jul 3, 2017

@bgeuken what do you think about the flash message? I had more in mind to set a hint into the recipient input field...

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jul 3, 2017

@ChrisBr I am not convinced this belongs to the input field. This is more about the recipients dropdown menu. When there are no event participants, the dropdown is disabled.

@ChrisBr

This comment has been minimized.

Member

ChrisBr commented Jul 3, 2017

@bgeuken Hmm, could also be an option!

@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Jul 4, 2017

I have pushed the changes

@coveralls

This comment has been minimized.

coveralls commented Jul 4, 2017

Coverage Status

Coverage remained the same at 92.003% when pulling 6294686 on nikhilgupta1211:selectinemail into 0fcf00e on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 4, 2017

Coverage Status

Coverage remained the same at 92.003% when pulling 6294686 on nikhilgupta1211:selectinemail into 0fcf00e on openSUSE:master.

@bgeuken

Looks good. Please fix the minor issues and then we can merge this.

states = (state) ->
$('#state'+state).click ->
users = $('#state'+state).data('users')
$('#event_email_to').val(users)

This comment has been minimized.

@bgeuken

bgeuken Jul 10, 2017

Member

Please don't use tabs for spacing. Just use whitespaces.

if $('#select_recp').data('users')
$('#select_recp').prop("disabled", true)
states = (state) ->

This comment has been minimized.

@bgeuken

bgeuken Jul 10, 2017

Member

Please use a function name that reflects what the function does.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jul 10, 2017

Please add the missing commit from #71 here. The other one includes the changes from this one anyway...

@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Jul 10, 2017

fixed it :)

@coveralls

This comment has been minimized.

coveralls commented Jul 10, 2017

Coverage Status

Coverage remained the same at 92.003% when pulling 1b3f636 on nikhilgupta1211:selectinemail into 220458c on openSUSE:master.

Added a placeholder for the event email 'to' field
Added coffeescript to change the placeholder when no recipients are present
Fixes #69
@coveralls

This comment has been minimized.

coveralls commented Jul 11, 2017

Coverage Status

Coverage remained the same at 92.003% when pulling c10438a on nikhilgupta1211:selectinemail into 220458c on openSUSE:master.

@bgeuken bgeuken merged commit c598ebb into openSUSE:master Jul 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 92.003%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment