Ingest Auto Refresh #713
Ingest Auto Refresh #713
Conversation
jameskerr
force-pushed
the
ingest-refresh
branch
2 times, most recently
from
ae65ef4
to
1c858e0
Compare
There was a problem hiding this comment.
Just finished checking this out and playing with it a bit. Looks great! I left a few small comments in code, but also wanted to throw out a couple of ideas which we may or may not want to address at this time:
-
I notice that the refresh notice will pop up, and if I dismiss it bc I'm mid-investigation of the existing space, then after a bit it will just pop up again and that might become a little annoying for users to repeatedly dismiss that notification. Perhaps if we offer a "don't remind me again" piece, which is a familiar ui interaction I've seen, then that would be one solution
-
The current behavior of auto-refreshing if there is no search can also be a little frustrating as I'm scrolling down the existing logs to get a general sense for what is there (and then it reloads and resets everything). It might not be clear enough that to stop that behavior you need to issue a search. Not sure what the solution is here, maybe a checkbox near the "Partial data available while loading... [x] auto-refresh enabled"? 🤷 Actually, could solve both problems with one stone if we have both that check box as well as a [refresh] button which will be disabled when it doesn't apply and enabled when it does, so that we don't need the notification at all.
| white-space: nowrap; | ||
| user-select: none; | ||
| background: white; | ||
| background: $gray-5; |
There was a problem hiding this comment.
I can never sneak a double background past you...
| @@ -30,6 +31,7 @@ export default () => { | |||
| let dispatch = store.dispatch | |||
|
|
|||
| initDOM("detail-root") | |||
| initUserInputClasses() | |||
| <XButton onClick={() => onClick(1)} /> | ||
| </div> | ||
| </div>, | ||
| lib.doc.id("tooltip-root") |
There was a problem hiding this comment.
Seems like we are using this "tooltip-root" element for more than just tooltips, and once we replace our use of <Tooltip /> with <BrimTooltip /> then it will not be used as a tooltip for any of these portal components. Maybe we can update the name?
|
@mason-fish thanks for the comments. Those are insightful. When I was testing, I found that the clicking the x was a bit annoying too. And I can see that if I scroll, it is also annoying. We can check to see if the user has scrolled down the page before auto refreshing. I do like your auto-refresh checkbox suggestion as well. That explicitly says what is happening. That might be my favorite idea. Would we turn off auto-refresh for them when you start typing or scrolling? I think this PR gets us at least 80% there so I'm inclined to merge it as is. But I would like to keep an eye on how people like it and remember these ideas here. |
space.ingest is now an object with progress and warnings add a Spaces.remove action to use after deleting a space always set defaults for the space in setDetail and setNames throw error if trying to update ingest progress of space that has no details yet
The tab history was being retained if a space was deleted after the user searched a few times. This happens when you reload the page. We rely on the tab history to know whether or not to do auto refresh on ingest.
|
@jameskerr Yeah totally, I think this is good to merge. Just wanted to share those ideas. Awesome work! |
fixes #616
If the tab history is empty, this component submits the current search. This
happens the first time this tab is rendered after a file is dragged in.
As the files are ingested the backend incrememnts a snapshot_counter to
indicate that more data is available to search.
Everytime space.ingest.snapshot is incremented, this component will effectively
hit the "Enter" key for the user if all these conditions are met:
If anyone of those is not true, then the component renders a message box to
allow the user to manually refresh if they want. This box appears when
The snapshot is acknowledged when any of these events happen: