Needs review
Project:
Advance Clock
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
12 Nov 2014 at 13:51 UTC
Updated:
9 Apr 2015 at 07:51 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
sharique commentedUpdated patch, did code clean up, Digital clock now working with moment js.
Comment #2
sharique commentedAnalog clock is working now.
Improved performance due removal theme_image for analog clock. Theme analog/digital clock only once.
In future we will remove images and use html5 canvas for rendering digital clock.
Comment #4
sharique commentedComment #5
sharique commentedI've removed the images from patch file, to fix apply failure. You need to manually replace image attached in the zip file.
Comment #6
sharique commentedOnce this issue committed #2372447: Moment timezone support, we can remove timezone js file from this module.
Comment #7
naveenvalechaAs I am not familiar with the advance_clock module code.Here area some small nutpicks that you need to do.
I have seen the lack of coding standards at various places.if u r using the sublime text,configure your editor using this blog post http://www.qed42.com/blog/sublime-awesome-sauce-drupal This really makes help me as well.
Its good to use the drupal_map_assoc instead.
A new line should be at the end of the file.
Add new line at the end of the file.
Use spaces instead of tabs.
Comment #8
naveenvalechaAs I am not familiar with the advance_clock module code.Here area some small nutpicks that you need to do.
I have seen the lack of coding standards at various places.if u r using the sublime text,configure your editor using this blog post http://www.qed42.com/blog/sublime-awesome-sauce-drupal This really makes help me as well.
Its good to use the drupal_map_assoc instead.
A new line should be at the end of the file.
Add new line at the end of the file.
.......
Use spaces instead of tabs.
Comment #9
sharique commented@Naveen, use the module, you will understand it.
Point 1 not done as it is not giving desirable output. #2372447: Moment timezone support is fixed so pt 4 is not applicable.
Also updated comment and help text.
Comment #10
naveenvalecha@Sharique,Thanks I'll see the code later.
#options => drupal_map_assoc(array(t('1'), t('2'), t('3'), t('4')));The both code will provide the same output.If its not would you please address me where I went wrong ?
Comment #11
sharique commentedI was using it wrong way.
Comment #12
naveenvalechaThanks! for rerolling :)
Comment #13
rahul.shindeComment #14
rahul.shindeConcat rather sending a string.
Add fallback value. please.
Dont do this data processing in tpl
Comment #15
rahul.shindeThis will have break when multilingual is on.
Comment #16
vijaycs85Looks like the status should be 'Needs work' as per #14 and #15.
Comment #17
sharique commentedHere is updated patch, please review.
Comment #18
sharique commentedIt looks like this modue is abandoned.
Comment #19
sharique commentedFork with this patch is created here https://github.com/sharique/advance_clock.
Comment #20
ravyg commented@Sharique module is not abandoned but there are too many changes to be looked at for now.
Please be patient as sometimes bandwidth is an issue for moderators, as drupal 8 is also on it's way.
Also there are a few unanswered questions :
1) Would existing users prefer this new library and if yes how can they update to this?
2) Also, are there any new features provided by this library, (Even if it's more supported) which this module uses and are not provided by old library?
3) Can we make this part as optional which might help old users (I need to have discussion on it).
So we can support both old and new users.
I would like this patch to be reviewed by 2 moderators as I know this is a crucial patch.
@rahulshinde @piyuesh23 on you own time please have a look.
I highly appreciate contribution but for such changes patience is equally appreciated.
Comment #21
sharique commentedWe have discussed few points, putting here with some additional points.
Moment library (http://momentjs.com) is very actively maintained as compared to jClocksGMT, hence I've chosen it. I agree that moment library size is bigger, which it is and also due to how library api module works, but use can still remove unnecessary file from that folder (ie, documentation, examples, etc )
I'm also trying to address following issue with this patch.
1. Moving js libraries out of module, so end use can choose the version based on his preferences.
2. jQuery specific version dependency.
3. Direct inclusion of external css (See http://cgit.drupalcode.org/advance_clock/tree/advance_clock.module?h=7.x...), changes there can create problem.
4. Inclusion of jquery from out of drupal ( See http://cgit.drupalcode.org/advance_clock/tree/advance_clock.module?h=7.x...), to me this is serious issue, this may break other parts of page, becuase they depend on the drupal or jquery_update provided version.
5. Fixing broken analog clock.
6. There is an memory leak issue in jClockGMT/jQuery.rotate (see https://github.com/mcmastermind/jClocksGMT/issues/5).
7. This is going to be version 2 so the people who want to use old library can use version 1 and those who want to use newer library version can use version 2.
8. I don't think it is good idea to have multiple libraries support such small module.
9. Both of us know that this module was written in hurry, so taking this opportunity to clean it up.
10. Using moment js we can provide localization support in future releases and may be more feature. (The one i'm interested in todo https://github.com/moment/moment/issues/1454, but can be done using moment js plug-ins).
11. Moment js have automates tests, so we ca expect better quality.