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.

Sign up for GitHub

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

Jump to bottom

Address REVIEW comments #78

Closed
pixelzoom opened this issue Jun 28, 2022 · 3 comments
Closed

Address REVIEW comments #78

pixelzoom opened this issue Jun 28, 2022 · 3 comments
Assignees
@marlitas
Labels
dev:code-review

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 28, 2022 •
edited
Loading

For code review #41 ...

During a code review, the reviewer has the option of either creating GitHub issues or adding REVIEW comments directly to the code.

REVIEW comments are typically minor things that do not require a GitHub issue, and they look like this example in MeanShareAndBalanceConstants.ts:

  //REVIEW These names are a little vague/incorrect. MOUSE_AREA_DILATION and TOUCH_AREA_DILATION dilation would be clearer.
  MOUSE_DILATION: 5,
  TOUCH_DILATION: 10,

There are currently 48 REVIEW comments, search for "REVIEW". Let me know if you have questions about any of them. Some them are suggestions, and a fine response would be "nah, not going to do that".

You can mention this issue in related commits. And feel free to close this issue when all REVIEW comments have been deleted.
@pixelzoom pixelzoom mentioned this issue Jun 28, 2022
Closed
74 tasks
marlitas added a commit that referenced this issue Jun 29, 2022
@marlitas
Address review comments, see: #78
f6d5674
marlitas added a commit that referenced this issue Jun 29, 2022
@marlitas
Address review comments, see: #78
a718fa8
samreid added a commit that referenced this issue Jul 1, 2022
@samreid
Omit 'phetioType' | 'phetioDynamicElement' from PipeModelOptions, see #…
c2d91f4 
…78
This was referenced Jul 1, 2022
Closed
Closed
@samreid
Copy link
Member

samreid commented Jul 1, 2022

For //REVIEW Needs more description - tell me about "water cup" in this sim., @marlitas added documentation in this commit:

a718fa8

Does that seem sufficient? We weren't sure if we should add more context or detail.

samreid added a commit that referenced this issue Jul 1, 2022
@samreid
Move REVIEW comment to issue comment, see #78
b6cffa3
@pixelzoom
Copy link
Contributor Author

... Does that seem sufficient?

Looks great, nice improvement.

marlitas added a commit that referenced this issue Jul 1, 2022
@marlitas
Change name of tickMarksNode, see: #78
b2c2d2c
marlitas added a commit that referenced this issue Jul 1, 2022
@marlitas
Strict Omit phetioType and phetioDynamicElement, see: #78
7ff1033
marlitas added a commit that referenced this issue Jul 1, 2022
@marlitas
Change name to PredictMeanSlider, see: #78
ee82d2d
marlitas added a commit that referenced this issue Jul 1, 2022
@marlitas
Rename to waterLevelTriangle, see: #78
6716149
marlitas added a commit that referenced this issue Jul 5, 2022
@marlitas
Add reentrant documentation, see: #78
de602c7
marlitas added a commit that referenced this issue Aug 5, 2022
@marlitas
Factor out ValveNode from PipeNode, see: #78
e5be158
marlitas added a commit that referenced this issue Aug 9, 2022
@marlitas
Omit transform options, see: #78
df4e78c
@marlitas
Copy link
Contributor

marlitas commented Aug 9, 2022

All review comments have been addressed. Closing.

@marlitas marlitas closed this as completed Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@marlitas marlitas

Labels
dev:code-review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@samreid @pixelzoom @marlitas