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 At.js support (removed Horsey and Banksy) #239

Merged
merged 13 commits into from Jan 31, 2019

Conversation

rexagod
Copy link
Member

@rexagod rexagod commented Jan 18, 2019

Fixes #225, #44

atjs_

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 grunt jasmine
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • 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

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@rexagod rexagod self-assigned this Jan 18, 2019
@rexagod rexagod added bug enhancement ready dependencies Pull requests that update a dependency file review-me Ask reviewers for help and removed bug ready labels Jan 18, 2019
@rexagod rexagod requested a review from jywarren January 18, 2019 22:27
@rexagod rexagod changed the title Added At.js support Added At.js support (removed Horsey and Banksy) Jan 19, 2019
@Rishabh-Kumar-Bothra
Copy link
Contributor

you seem to have committed bower_components. Shouldn't it be added to .gitignore
Thank you

@rexagod
Copy link
Member Author

rexagod commented Jan 19, 2019

@geekychaser Good call. 👍 @jywarren Do we need webgl-distort? Also currently there are two repository properties in bower.json.

"repository": {
"type": "git",
"url": "https://github.com/jywarren/webgl-distort.git"
},

"repository": {
"type": "git",
"url": "git+https://github.com/publiclab/PublicLab.Editor.git"
},

@rexagod
Copy link
Member Author

rexagod commented Jan 19, 2019

@publiclab/reviewers

@jywarren
Copy link
Member

Whoa certainly don't need webgl-distort, good catch!

I'd be happy to accept a change to the repository properties. Is this one of the libs where we are using Bower still or should we be deprecating it for a pure npm package.json approach?

This is looking great! Do I see a bit of delay in the autosuggestion popping up? I'm guessing it's local and static in the example, any idea why that might be slow?

I'd like to potentially remove the "suggestions" line to keep it simple. Is that ok?

Thanks!!! Very cool!

@rexagod
Copy link
Member Author

rexagod commented Jan 21, 2019

Hi @jywarren! Hope you're doing well. I have removed webgl-distort from bower.json, and the Suggestions heading from both "@" callouts and "#" hashtags as well.

I did think of using npm for this, since bower has been deprecated, but the README suggested using Component or Bower to install At.js and the underlying Caret.js, maybe because the package isn't maintained anymore. If you're interested, I can try doing a npm implementation for this and verify it on my local in a pure npm approach and let you know.

As for the delay, other than my network being a bit slow at the time of recording this, was due to the large number of users (300k+, as you mentioned here) currently in our database which hinders the API response time and also increased the "@gauravano" fetch time in the above example (as it occurs on a high speed connection too, and I have doubly checked this). This normally does not happen for hashtags, since the notes are pretty low in number right now, so a GET request gets through pretty fast, but this can be fatal for the response time as the number of notes increase in the near future. The same goes for other resources as well. Please refer to my issue below for a better (visual) explaination, and @milaaraujo's issue that rounds up all the similar issues and resources related to the slow API problem.

My issue (response difference between @ and #): publiclab/plots2#4670
@milaaraujo's issue: publiclab/plots2#4561

@jywarren
Copy link
Member

jywarren commented Jan 22, 2019 via email

@jywarren
Copy link
Member

Ok, yes I think we had better use npm here, I'm sorry to say. In general we've been phasing out Bower slowly on all repositories.

Thanks @rexagod!

Also I see a lot of small formatting changes like double/single quotation marks. Was that just tidying up? Thanks!

@rexagod
Copy link
Member Author

rexagod commented Jan 30, 2019

@jywarren Shifted to NPM. Also, the small formatting changes were due to Prettier, it stresses on using the double quotes for some reason.

@rexagod rexagod added ready and removed review-me Ask reviewers for help labels Jan 30, 2019
@rexagod
Copy link
Member Author

rexagod commented Jan 30, 2019

@jywarren @gauravano Can we merge this now?

@jywarren jywarren merged commit 79f331f into publiclab:master Jan 31, 2019
@jywarren
Copy link
Member

Fantastic work @rexagod !!! Shall we bump the version number by a 0.x.0 minor value? Awesome!!!!!! 🎉 🎉 🎉

@rexagod
Copy link
Member Author

rexagod commented Jan 31, 2019

Definitely @jywarren! I think we can surely introduce a minor version bump at this point, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

At.js refactoring and fixes
3 participants