-
Notifications
You must be signed in to change notification settings - Fork 71
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
issue/644 - Issue navigation not always working #853
Conversation
This change causes a different issue. I fixed issue 121, and after fixing, it became issue 116. I fixed the next issue, and it also became 116. That ID seems to be the first issue for that particular content item (a page in this case). I switched back to the production code and reset the test course, and I get a different problem. When I fix issue 121, it becomes issue 1. |
Thanks for testing. I'll try it out more tomorrow. I didn't test the edge
cases or fixing issues while navigating. Seemed to good/easy to be true.
But I'm at least able to replicate this more frequently now.
…On Mon, Oct 17, 2022, 3:00 PM Jacob Bates ***@***.***> wrote:
This change causes a different issue. I fixed issue 121, and after fixing,
it became issue 116. I fixed the next issue, and it also became 116. That
ID seems to be the first issue for that particular content item (a page in
this case).
I switched back to the production code and reset the test course, and I
get a different problem. When I fix issue 121, it becomes issue 1.
—
Reply to this email directly, view it on GitHub
<#853 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAGWN6E5H3JXTGIHPM5H3DWDW45BANCNFSM6AAAAAARBXDAIQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
From what I can see the problem you describe seems to be there with or without the fix on this PR. Resolving issues and then performing navigation introduces issues. In both cases when you click resolve it marks the issue as resolved but seems to leave it in the navigation list. However when you click resolve on another issue it removes the previous resolved issue and sometimes causes the order to be messed up, occasionally jumping around the list or causing navigation to be unpredictable. I feel like ideally it should just leave the resolved issues in the filtered list until the modal is closed. This might not be easy though because I think this modal is connected to the filter? To get this to work we'd probably have to retain the state of the filtered list until the modal is closed then update the list? This sounds complicated. The alternative would be to remove the resolved issue immediately after it's resolved and update all of the state. This would make it so it couldn't be easily un-resolved though if it was mis-clicked. |
Also with the code "as-is" it gets into a state when you can easily get a 404 error back from the API after a few attempts to resolve/navigate and un-resolve. It typically ends up back on issue 1 and if you resolve that it gets an error because the list is wrong. I might try to implement the alternative of just removing it immediately. But this is seeming pretty complex.
|
So here's the problem I'm seeing with this and I'm getting closer filteredRows are getting updated every render cycle with
As issues are resolved, this FilteredContent is being updated, it's not static. This is messing up the list. It looks like it's a problem with the code that checks the recentlyUpdated and recentlyResolved status and is removing it from the list. This filter seems problematic to me to use as it's navigating through the list when Active is the default filter checked and it doesn't actually update until the second action is taken. I feel like maybe this can be improved if maybe Active isn't checked by default and if it is the issue that's resolved is removed from the list immediately. I'm not sure why it isn't quite yet.
|
I feel working on this that there are two separate issues and this fixes the one related to navigation not working correctly. The other one @bagofarms mentioned related to resolving/fixing issues and navigation is present in some form with or without this fix. We are also looking at that. |
I'm going to close this one as there's a lot more to getting this to work and when/if a fix is implemented it won't be on this PR. |
Fixes #644. This seems to be working reliably for me in initial testing. I wonder if this was the initial idea for this @bagofarms? It looks like this has been in there for 2 years from a commit by @webchuckweb. I'm surprised you haven't seen it come up?