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

Jg0101 can remove user from room with admin #45

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jdg006
Copy link

@jdg006 jdg006 commented Apr 15, 2018

let me know if you want me to change anything.

@jdg006
Copy link
Author

jdg006 commented Apr 15, 2018

Are the travis ci tests failing for anyone else or just me? I ask this because I am getting errors from files that were not in my pull request.

@valterbarros valterbarros self-requested a review April 15, 2018 12:59
@valterbarros
Copy link
Owner

Ok's man I will review your code, Thank's

@andersonfernandes andersonfernandes self-requested a review April 16, 2018 12:47
@valterbarros
Copy link
Owner

@jdg006 Actually github request to update some dependency on gemfile from project, and I need do this on master, one of the affected dependencies was rails-html-sanitizer, in the travis log he ask to you run bundle update rails-html-sanitizer. Try this and tell what happen.

Copy link
Owner

@valterbarros valterbarros left a comment

Choose a reason for hiding this comment

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

Hey guy I like your code you did test woow awesome. But all actions from rooms are make via js or ajax, and in this code your remove user and redirect page to rooms_path this behavior is not cool. Can you do this remove without reload the page? Thank's if you have some doubts ask me.

@jdg006
Copy link
Author

jdg006 commented Apr 20, 2018

let me know if you want any other changes or if I need to rearrange any of the coffeescript files.

Copy link
Owner

@valterbarros valterbarros left a comment

Choose a reason for hiding this comment

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

I test in my machine and is missing only little details. When you remove user if it is success I think is necessary return a response for user like a notification. To do this you can create a remove_user.js.erb to render a new notification with js helper new AppNotification(message, 2000).show_success_with_timeout() Remember this message need be a locale inside cofing/locales/en and config/locales/pt-br

en message: User removed with success
pt-br message: Usuário removido com sucesso

@jdg006
Copy link
Author

jdg006 commented Apr 21, 2018 via email

@andersonfernandes andersonfernandes removed their request for review March 22, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants