Closed Bug 883148 Opened 9 years ago Closed 9 years ago

Refactor page.js value and fix typo in comment

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jfong, Assigned: jfong)

Details

Attachments

(1 file)

I saw a couple of calls to the value 4 in page.js and thought it would be nice to refactor this to a constant. Also noticed a small typo on a comment.
Attached file Pull request url
Attachment #762670 - Flags: review?
Attachment #762670 - Flags: review? → review?(21)
Comment on attachment 762670 [details]
Pull request url

Hi,

   Sorry for the r- but although I am comfortable with your idea, the implementation is wrong:

   There are two different constants:

1) img.width = MAX_ICON_SIZE + 4 * SCALE_RATIO; -> ICON_PADDING_IN_CANVAS

2) var x = node.dataset.posX = parseInt(node.dataset.posX || 0) +
                      ((Math.floor(to % 4) - Math.floor(from % 4)) * 100);
                      ((Math.floor(to % ICON_MAX) - Math.floor(from % ICON_MAX)) * 100); --> ICONS_PER_ROW

Please fix it and you can ask me for a new review, thanks a lot!!
Attachment #762670 - Flags: review?(crdlc) → review-
Comment on attachment 762670 [details]
Pull request url

updated the pull request - let me know if this one is better. thanks!
Attachment #762670 - Flags: review- → review?(crdlc)
Comment on attachment 762670 [details]
Pull request url

The patch is ok for me but before merging, please review lint errors:

----- FILE  :  /home/travis/build/mozilla-b2g/gaia/apps/homescreen/js/page.js -----
Line 320, E:0110: Line too long (89 characters).
Line 321, E:0110: Line too long (91 characters).
Line 759, E:0110: Line too long (98 characters).
Line 761, E:0110: Line too long (98 characters).
Found 4 errors, including 0 new errors, in 1 files (803 files OK).

Great work!
Attachment #762670 - Flags: review?(crdlc) → review+
Updated the pull request with the fixes - thanks!
https://github.com/mozilla-b2g/gaia/commit/e286239642619db108ece49f4e0ea4970cdff2d3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.