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.
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
feat: auto-answer one at a time, not into the future #3764
feat: auto-answer one at a time, not into the future #3764
Changes from 4 commits
30139f2
3911c82
63db51a
fddef39
0078c28
32da380
d8ff09c
0495d9e
a1339c1
ccac584
89ad20b
65cfd79
37b2f74
c917afd
95c28af
19465b1
59bf5d9
9cc07d2
fadcf03
81bd7f0
9475e74
a77cca5
348b851
738ff99
65d8375
7bba548
9b522e6
efe731d
570cb28
91c9dfd
a60b5aa
d9c94cf
d699da6
d96476c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Higher level comment, just discussed on dev call.
This repeated logic may be best at the level above this (
Node.tsx
). It's possible we could pass an prop into each component viagetComponentProps()
- maybeisAutoAnswered
orisVisible
or something meaningful.This would allow us to have simpler components and more centralised logic over visibility. The visibility logic would be component specific in some circumstances - for example, filters and checklists have different requirements. I think this would make it feel like checking for visibility/auto answering is a global display rule of the graph, with many implementations (which it is I belive), as opposed to a series of exceptions sort of hidden away within each component.
Worth a look as a follow up PR to this once, or at least worth another look before we jump into applying this pattern to other component types I think.
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.
Very relevant questions & agree there's pieces of code here that already indicate that
Node.tsx
is probably the better place to handle this (eg the initial guard clauses in the store methods you point out), but very in favor of exploring in a follow-up PR to this one because I think it's a larger question than only components I've touched here!For example:
Let's keep this one "unresolved" & experiment with this before we try out input automations soon 👍