-
Notifications
You must be signed in to change notification settings - Fork 337
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
Ensure maxframes setting is respected #1569
Conversation
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.
Nice to get this rename done as well, just have a minor comment to clean up a comment and a suggestion
* | ||
* This file is part of Neo4j. | ||
* | ||
* Neo4j is free software: you can redistribute it and/or modify |
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.
Nice to see this moved to an e2e test!
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.
+1 on loving seeing an e2e test for this! I do wonder though, it seems we're only testing a decrease of the limit, and maybe we should we add an extra test where we increase the limit? wdyt?
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.
Good idea Jon, I'll add that.
@@ -193,8 +195,9 @@ function setRecentViewHelper(state: FramesState, recentView: FrameView) { | |||
} | |||
} | |||
|
|||
function ensureFrameLimit(state: FramesState) { | |||
const limit = state.maxFrames || 1 | |||
/** TODO **/ |
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.
Leftover comment
const limit = state.maxFrames || 1 | ||
/** TODO **/ | ||
function ensureFrameLimit(state: FramesState, maxFrames: number) { | ||
const limit = maxFrames || 1 |
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.
Perhaps we could require maxFrames be larger than 0 here, rather than have a default value? WDYT?
nodePropertiesExpandedByDefault: boolean | ||
} | ||
|
||
export const initialState: FramesState = { | ||
allIds: [], | ||
byId: {}, | ||
recentView: null, | ||
maxFrames: 30, |
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.
Oh nice, at first I was questioning why the default/initial was removed, by now I see it was a duplicate of the default/initial from settings! Really great that you've cleaned this up!
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.
Great work on this! Let's merge once the tests go green :-)
No description provided.