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

Pop up menu is not updated without page refresh #3810

Closed
wants to merge 10 commits into from

Conversation

tsparksh
Copy link
Member

Fixes #3370 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@plotsbot
Copy link
Collaborator

plotsbot commented Oct 27, 2018

2 Messages
📖 @thesparks Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@tsparksh
Copy link
Member Author

tsparksh commented Oct 27, 2018

@igniteeng000, @SidharthBansal, @jywarren please check.

like.js code is so strange :/ (changelikecount() and changenotlikecount() 😕 )

@tsparksh
Copy link
Member Author

@tech4GT

@SidharthBansal

This comment has been minimized.

@tech4GT
Copy link
Member

tech4GT commented Oct 27, 2018

@thesparks Sidharth is right, please try to keep the conversations as nice as possible😄
As for the code, I'll have a look. Thanks!

@tsparksh
Copy link
Member Author

Sorry.
Any news about task?

@tsparksh
Copy link
Member Author

???

@tech4GT
Copy link
Member

tech4GT commented Oct 30, 2018

@thesparks I'm sorry for the delay, I've been very busy with my university work for the past week, maybe @jywarren can help here. Thanks!

@jywarren
Copy link
Member

Ah, this is a pretty interesting implementation! Could you try to demonstrate how it works in a gif, following these steps?

#3513

That helps us understand the UI sequence and flow. Thank you!

@jywarren
Copy link
Member

Also, Code Climate is asking us to reduce the redundancy of the code. I think it's a good suggestion -- take a look! https://codeclimate.com/github/publiclab/plots2/pull/3810

@tsparksh
Copy link
Member Author

tsparksh commented Nov 9, 2018

deepin-screen-recorder_select area_20181109200121

@jywarren, I am sorry, I did not have time these days. I tried to remove the codeclimate-warn, but this code was written before me and I do not know how to fix it

@tsparksh
Copy link
Member Author

@avsingh999 please check this too

@tsparksh tsparksh closed this Nov 15, 2018
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.

5 participants