-
Notifications
You must be signed in to change notification settings - Fork 129
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
QueryLibrary model and reducer #1239
Conversation
Signed-off-by: Mason Fish <mason@looky.cloud>
96a1412
to
a086573
Compare
|
||
export default { | ||
getRaw: (state: State): QueryLibraryState => state.queryLibrary | ||
} |
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.
We may want more selectors here, and will likely return the library as a parsed tree model in one of them. For now I am only returning the raw state and the tests are all built off of that, and as I am wiring this up to the components that consume this data then I may add more
src/js/state/QueryLibrary/initial.ts
Outdated
const init = (): QueryLibraryState => ({ | ||
id: "root", | ||
name: "root", | ||
items: [] |
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.
this will actually be populated by #1201
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.
Mason, this is a great foundation. I'm glad we have those thorough tests as well. I have 2 questions for ya below.
src/js/state/QueryLibrary/reducer.ts
Outdated
// cause an off by one issue since the destination index will be affected after | ||
// removal (e.g. an item cannot be moved to the end of its current group because of this). | ||
// For this situation we instead remove the item first, and then insert its copy | ||
if (srcItemPath.length === destItemPath.length) { |
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.
Is this just comparing the depths? Should check to see if the two paths (minus the last item) are equal?
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 thank you, yes this is not enough as is. will fix
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.
actually, this does work as is: removing before or after only matters if it IS the same group. But I still prefer the handling to be only during the special case so am adding an isEqual check in the next commit
src/js/state/QueryLibrary/types.ts
Outdated
export interface Query { | ||
id: string | ||
name: string | ||
value: string |
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.
What do you think about naming this field zql
instead of value
?
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.
Sure, that works
Signed-off-by: Mason Fish <mason@looky.cloud>
Signed-off-by: Mason Fish <mason@looky.cloud>
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.
I was thinking about this this week and wondered if the QLIB prefix was a bit too jargony. I think a prefix like QUERIES would be more obvious for a new user.
QUERIES_ADD_ITEM
QUERIES_REMOVE_ITEM
QUERIES_MOVE_ITEM
etc..
src/js/state/QueryLibrary/reducer.ts
Outdated
@@ -71,7 +71,7 @@ const moveItem = ( | |||
// cause an off by one issue since the destination index will be affected after | |||
// removal (e.g. an item cannot be moved to the end of its current group because of this). | |||
// For this situation we instead remove the item first, and then insert its copy | |||
if (srcItemPath.length === destItemPath.length) { | |||
if (isEqual(initial(srcItemPath), initial(destItemPath))) { |
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.
I didn't know about that "initial" function. Very cool
@jameskerr I agree qlib is a very insider term. I'm only using it in code for var names and for the action strings, and never show it to users. I shortened it to save some typing and I think it's mostly used within folders that have the name expanded so the context for future devs should be clear, but if you feel strongly I can change |
Thanks for your comment Mason. You know, it appears I do feel strongly enough to request the change to be QUERIES. I think the top-level folder in the state should also match and just be "queries". Thanks! |
Signed-off-by: Mason Fish <mason@looky.cloud>
Thanks for making that name change for me! |
fixes #1198
Adds a new QueryLibrary reducer. This code is all additive and is not yet being used, though it will be once we link together the remaining ui work tracked in #1199
The underlying data structures we are using to represent this lib are simply an object tree and arrays. The arrays provide the order which we need while also keep the structure easily serializable into JSON, and the tree structure provides the grouping hierarchy which we will want to use later on. The issue/inefficiency of re copying elements on array resize/reorder/remove is moot since we are using redux, which depends on data being immutable and when changes occur they must be done by creating new resources anyway.
Signed-off-by: Mason Fish mason@looky.cloud