-
Notifications
You must be signed in to change notification settings - Fork 2
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
about_comments #19
base: main
Are you sure you want to change the base?
about_comments #19
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ Guidelines and best practices used by Eventbrite to provide consistency and prev | |
0. [Conditionals](#conditionals) | ||
0. [Assignments](#assignments) | ||
0. [Naming Conventions](#naming-conventions) | ||
0. [Comments](#comments) | ||
|
||
## Conditionals | ||
|
||
|
@@ -410,7 +411,53 @@ _superComplextName: function() { | |
UpdateAll: function() { | ||
/ /code here | ||
} | ||
``` | ||
## comments | ||
|
||
### commented code | ||
|
||
*We recommend not to leave commented code lingering around. If the code is meant to be used in the future, either save the code on a branch or in our internal paste bin via phabricator. (arc paste).* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we mention arc & phabricator since it's internal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well phab is public. the product/software. |
||
|
||
### when the comment has multiple lines we use the block form (i.e: JSDocs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should read like:
Is this saying that whenever we have a comment that spans multiple lines it has to be in JS doc form? As in this: /**
*/ vs. /*
*/ |
||
|
||
```js | ||
|
||
/** | ||
* returns the calculated offset for lower bound range. (from) | ||
* @example | ||
* // returns 2 | ||
* scenario: | ||
* Viewport Large | ||
* skiping first and applying corrections | ||
* @returns {integer} lower bound range | ||
*/ | ||
|
||
const getMinBound = (pageSize = constants.DEFAULT_PAGE_SIZE) => ( | ||
pageSize === constants.DEFAULT_PAGE_SIZE ? constants.MIN_BOUND_SMALL : constants.MIN_BOUND_DEFAULT | ||
); | ||
``` | ||
|
||
### when the comment has a single line we instead one liner comments: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
```js | ||
const getToOffset = (pageNumber, middleRangeOffset, maxPagesShown, maxBound) => { | ||
// default -> page is in the middle, and we have enough pages in both sides. | ||
let to = pageNumber + middleRangeOffset; | ||
|
||
// are we too close to the bottom? if so, we need to adjust the split | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this comment is on two lines but uses one-liner comments? Should it have used a different style? |
||
// relative checks where the split doesn't work (sorry van damme) | ||
if (to < maxPagesShown + 1) { | ||
to = pageNumber + maxPagesShown; | ||
} | ||
|
||
// hard bound to the top | ||
if (to > maxBound) { | ||
// on small + 1 | ||
to = maxBound; | ||
} | ||
|
||
return to; | ||
}; | ||
``` | ||
|
||
**[⬆ back to top](#table-of-contents)** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to mention about how inline comments are prohibited and why? You can also link to the eslint rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please capitalize here and below