flake8
passed.
JSHint
passed.
Fixed the Update -> Add File review request action not responding to clicks.
Review Request #9207 — Created Sept. 22, 2017 and submitted
In the Review Request screen, clicking Update -> Add File would not do
anything.The Update element's click event is registered before the Add File
element's click event. As a result, the Update element's click event is
firing before, eating up the Add File's click event and thus causing the
upload file dialog to never appear.The
onUploadFileClicked()function was moved fromReviewablePageViewto
ReviewRequestEditorView, and the element's handler registered in the
setupActions()function. The.has-menuhandler was moved to
ReviewablePageView, better respecting the ownership structure of the
page. This changes the order in which the click events are fired, allowing
theonUploadFileClicked()function to actually fire in response to the
Add File element being clicked.Furthermore, the
has-menuhandler was moved from
ReviewRequestEditorViewtoReviewablePageViewAfter the fix, the upload file dialog appears as intended.
Ran JSTests.
Tested manually by clicking Update -> Add File, where the upload file
dialog appears. This was done on both computer and mobile browsers.
| Description | From | Last Updated |
|---|---|---|
|
Also the description should always wrap at 70-75 characters. I recommend writing the description in your commit message. Many editors … |
 chipx86
|
|
|
The description doesn't make it clear what the source of the problem was or how this fixes it. Descriptions should … |
chipx86
|
|
|
Missing period in the second-to-last paragraph of the description. |
chipx86
|
|
|
For the summary, "in Review Request" is kind of akwardly-worded and not quite correct. I'd suggest the summary be: Fixed … |
chipx86
|
|
|
"This method displays a popup..." |
 brennie
|
|
|
Only one space between sentences. We're not using typewriters here! |
 david
|
|
|
The reviewRequest variable is only used once, so we could remove the variable definition and juts move the model.get call … |
david
|
|
|
Undo this. |
brennie
|
|
|
Lets accept e (type Event) as an argument and call e.stopPropagation() and e.preventDefault() instead. |
brennie
|
|
|
Trailing whitespace |
brennie
|
|
|
Care to update this to use e.preventDefault() and e.stopPropagation() ? It too can be done at the top of the … |
brennie
|
|
|
No longer true. This function doesn't return anything. |
brennie
|
|
|
Mind updating this function to use preventDefault and stopPropagation too? |
brennie
|
|
|
No longer true. This function doesn't return anything. |
brennie
|
|
|
Mind adding Args to all these functions? |
brennie
|
|
|
Undo this. |
brennie
|
|
|
This is on the wrong class. It needs to be on RB.ReviewablePageView. Can you move it, and then we'll need … |
chipx86
|
|
|
This one can do a return false and get rid of e. It's equivalent of these commands. The reason we'd … |
chipx86
|
-
-
The description doesn't make it clear what the source of the problem was or how this fixes it. Descriptions should always tell a story of what the reported problem was, why it happened, and how it was fixed. Write it like you're telling someone what you did so they can write a report on it.
As far as I can tell, the event handler really should live on the page's view, not the
ReviewRequestEditorView, so I'm not sure why it was moved. What was the source of the problem?
NI
- Change Summary:
-
Updated description to be more specific.
- Summary:
-
Fixed Update -> Add File not doing anything in Review RequestFixed Update -> Add File not doing anything in Review Request.
- Description:
-
~ When clicking Update -> Add File would not do anything.
~ In the Review Request screen, clicking Update -> Add File would not do
+ anything. ~ The onUploadFileClicked() function was moved from reviewablePageView.es6.js to reviewRequestEditorView.es6.js, and the element's click function set to it.
~ The Update element's click event is registered before the Add File
+ element's click event. As a result, the Update element's click event is + firing before, eating up the Add File's click event and thus causing the + upload file dialog to never appear. + + The onUploadFileClicked() function was moved from reviewablePageView.es6.js
+ to reviewRequestEditorView.es6.js, and the element's click function set to + it from the setupActions() function. This changes the order in which the + click events are fired, allowing the onUploadFileClicked() function to + actually fire in response to the Add File element being clicked. + + After the fix, the upload file dialog appears as intended.
- Testing Done:
-
Ran JSTests.
~ Tested manually by clicking Update -> Add File, where the upload file dialog appears.
~ Tested manually by clicking Update -> Add File, where the upload file
+ dialog appears. - Commit:
-
1d2b98d473d4b57b818a35d412b4a81a9181eefdc6825d1f055af43d0a1dd74fac6c50b9fa8df57e
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
NI
- Change Summary:
-
Moved the Upload File handler to
ReviewRequestEditorView, and the.has-menuhandler toReviewablePageView. - Description:
-
In the Review Request screen, clicking Update -> Add File would not do
anything. The Update element's click event is registered before the Add File
element's click event. As a result, the Update element's click event is firing before, eating up the Add File's click event and thus causing the upload file dialog to never appear. ~ The onUploadFileClicked() function was moved from reviewablePageView.es6.js
~ to reviewRequestEditorView.es6.js, and the element's click function set to ~ it from the setupActions() function. This changes the order in which the ~ click events are fired, allowing the onUploadFileClicked() function to ~ actually fire in response to the Add File element being clicked. ~ The
onUploadFileClicked()function was moved fromReviewablePageViewto~ ReviewRequestEditorView, and the element's handler registered in the~ setupActions()function. The.has-menuhandler was moved to~ ReviewablePageView, better respecting the ownership structure of the~ page. This changes the order in which the click events are fired, allowing + the onUploadFileClicked()function to actually fire in response to the+ Add File element being clicked. + + Furthermore, the
has-menuhandler was moved from+ ReviewRequestEditorViewtoReviewablePageViewAfter the fix, the upload file dialog appears as intended.
- Commit:
-
c6825d1f055af43d0a1dd74fac6c50b9fa8df57e55c111a06f5c03487600432c88ed1b8fbc0faf89
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
NI
- Change Summary:
-
Removed extra space in documentation.
- Commit:
-
55c111a06f5c03487600432c88ed1b8fbc0faf899ef4f4c6fe10d04b99f91f7488f989e28d739bea
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
NI
- Change Summary:
-
Removed redundant variable, readded line break that was accidentally removed and changed how the event propagation was being stopped in
onUploadFileClicked. - Commit:
-
9ef4f4c6fe10d04b99f91f7488f989e28d739beaed9de0f8a786ba4a6ccb9aaab81a8b28e27c0a71
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
NI
- Change Summary:
-
Removed trailing whitespace, and changed how event propagation is consumed in
onShipItClicked. - Commit:
-
ed9de0f8a786ba4a6ccb9aaab81a8b28e27c0a712535d454f6bd21bbf5c7a43b683ae9be474d0ecd
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
NI
- Change Summary:
-
Changed how
onMenuClickedstops event propagation, removed documentation on return values for all functions that no longer return false to consume events. - Commit:
-
2535d454f6bd21bbf5c7a43b683ae9be474d0ecd5e9d94e75d4452bc0867cbb4776a463bcdfa7e2c
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
NI
- Change Summary:
-
Added args to all functions which were changed to take an Event as a parameter.
- Commit:
-
5e9d94e75d4452bc0867cbb4776a463bcdfa7e2c158f8eba460f111e0395b3255ca04c5a02016347
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
This is on the wrong class. It needs to be on
RB.ReviewablePageView. Can you move it, and then we'll need to perform more strict testing.This particular event handler is from commit 7bfd3ae3, which says:
Fix clicking on actions for review requests.
For mobile usage, the actions now live in a top-level
<li>with a
"has-menu" class. This was getting intercepted by an event handler that
was blocking clicks from triggering links. The event selector now points
to menus within that class.So what you need to do is active the browser's mobile mode. Set it for something like an iPhone 6S size. Reload the page. Then try clicking on actions within the menu. You'll definitely want to try actions a couple levels down, like the Close -> Submitted. Test this both before and after the proper fix.
You should also try using your phone, along with the above test, to make sure it works on mobile devices. You can get the IP of your laptop and try using it instead of
localhostin the browser. You might need to configure your firewall appropriately, if it's interfering.
NI
- Change Summary:
-
Moved .has-menu click event registration to
RB.ReviewablePageView. - Summary:
-
Fixed Update -> Add File not doing anything in Review Request.Fixed the Update -> Add File review request action not responding to clicks.
- Testing Done:
-
Ran JSTests.
Tested manually by clicking Update -> Add File, where the upload file
~ dialog appears. ~ dialog appears. This was done on both computer and mobile browsers. - Commit:
-
158f8eba460f111e0395b3255ca04c5a0201634730c6b6892080b96915449b4e77476e2a5b9cb103
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
This one can do a
return falseand get rid ofe. It's equivalent of these commands. The reason we'd specifically call them is to ensure that they're called before any operations we care about occur, in case ours crash. In this particular case, we don't run any special code, so we can justreturn falseand let jQuery do its thing.