facebookarchive / shimmer-android

An easy, flexible way to add a shimmering effect to any view in an Android app.
http://facebook.github.io/shimmer-android/
Other
5.32k stars 695 forks source link

Small comments #1

Closed romainpiel closed 6 years ago

romainpiel commented 9 years ago

Hi, I basically did this PR to give you some feedback on this library. This approach is generally looking very good, it's pretty nice to affect this behaviour to any kind of view. Now a few comments:

Anyway, good job. Keep up the good work!

facebook-github-bot commented 9 years ago

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

facebook-github-bot commented 9 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

willbailey commented 9 years ago

whoops wrong thread :/

jasleensingh commented 9 years ago

Thanks, these were good comments. I don't want to merge this PR yet, as:

  1. I've updated the minSDK to 11, instead of 16. As a result, you should update your patch to make the newer method (removeOnGlobalLayoutListener) protected by a build version check (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN).
  2. BuildConfig was not generated if the library was compiled using Buck. I've updated the config file so that it would work. Can you do a sanity check of your patch by building with Buck?
  3. I think you're right about the the unnecessary gc and try. Though, we should better handle the situation if it arises (by precomputing memory requirements for example).
  4. The try/finally is more of a stylistic thing - Android framework classes read attribute values without enclosing them in such a block, which seems reasonable as we don't expect an exception to be thrown here.

Apologies for the late reply.

ghost commented 9 years ago

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.