This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Avoid serializing Function in array template #13432
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bridiver
reviewed
Mar 13, 2018
docs/state.md
Outdated
| }], | ||
| top: number // the top position of the context menu | ||
| }, | ||
| contextMenuDetail: string, // uuid for triggering state update |
There was a problem hiding this comment.
I think we should keep the docs here with a note explaining the hack
bridiver
reviewed
Mar 13, 2018
js/actions/windowActions.js
Outdated
| */ | ||
| setContextMenuDetail: function (detail) { | ||
| const Immutable = require('immutable') |
There was a problem hiding this comment.
please leave a comment explaining why we did this
bridiver
reviewed
Mar 13, 2018
js/stores/windowStore.js
Outdated
| @@ -641,7 +648,7 @@ const doAction = (action) => { | |||
| break | |||
| case windowConstants.WINDOW_TOGGLE_MENUBAR_VISIBLE: | |||
| if (getSetting(settings.AUTO_HIDE_MENU)) { | |||
| doAction({actionType: windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL}) | |||
| windowState = contextMenuState.setContextMenu(windowState) | |||
There was a problem hiding this comment.
there is some weird reason why these call the action instead of just setting state. To be safe I think we shouldn't change it unless we have to
bridiver
reviewed
Mar 13, 2018
| @@ -215,7 +218,7 @@ class ContextMenu extends React.Component { | |||
| const currentWindow = state.get('currentWindow') | |||
| const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map() | |||
| const selectedIndex = currentWindow.getIn(['ui', 'contextMenu', 'selectedIndex'], null) | |||
| const contextMenuDetail = currentWindow.get('contextMenuDetail', Immutable.Map()) | |||
| const contextMenuDetail = contextMenuState.getContextMenu() | |||
There was a problem hiding this comment.
we should still pass in the state even though we aren't using it. That will simplify things if we fix the real issue later.
There was a problem hiding this comment.
the idea is to hide the hack from everything except the action dispatch
bridiver
reviewed
Mar 13, 2018
app/common/state/contextMenuState.js
Outdated
| getContextMenu: (windowState) => { | ||
| windowState = validateState(windowState) | ||
| return windowState.get('contextMenuDetail', Immutable.Map()) | ||
| getContextMenu: () => { |
bridiver
reviewed
Mar 13, 2018
app/common/state/contextMenuState.js
Outdated
| if (windowState.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') { | ||
| windowState = windowState.set('hamburgerMenuWasOpen', true) | ||
| if (contextMenuDetail.get('type') === 'hamburgerMenu') { | ||
| hamburgerMenuWasOpen = true |
There was a problem hiding this comment.
we could leave this in the state since it's just true/false
Codecov Report
@@ Coverage Diff @@
## 0.21.x-C65 #13432 +/- ##
==============================================
- Coverage 57.1% 57.08% -0.02%
==============================================
Files 280 280
Lines 27495 27507 +12
Branches 4470 4471 +1
==============================================
+ Hits 15700 15702 +2
- Misses 11795 11805 +10
|
fix #13421 fix #11576 Auditors: @bridiver Test Plan: 1. Set `sessionSaveInterval` to `1` in js/constants/appConfig.js 2. Playing around with bookmark toolbar folder 3. Playing around with auotfill menu(fill with entry and clear) 4. Playing around with hamburger menu 5. Playing around with back/forward long pressed menu 6. There shouldn't be any exceptions about "function can be cloned"
|
addressed all the comments |
bsclifton
approved these changes
Mar 14, 2018
This was referenced Mar 14, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix #13421
fix #11576
Auditors: @bridiver
Test Plan:
sessionSaveIntervalto1in js/constants/appConfig.jsSubmitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests