Skip to content
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

Keep recently changed issues in UFixitModal (#887) #892

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ssciolla
Copy link
Contributor

@ssciolla ssciolla commented Feb 23, 2023

This PR aims to resolve #887. It is still a draft.

The changes here make it so multiple issues that have been fixed or resolved remain in the open UFixitModal. (In constrast, the code in main only keeps the most recently changed issue in the modal.) This is intended to assist users in reviewing their work (or potentially un-doing resolves, though that is currently problematic, see #867). It also prevents indexes in the bottom-left of the modal from shrinking by one (because previously fixed or resolved issues aren't being filtered out).

The recent state of the issues is set to false when the modal is closed; thus these "recently updated" issues will not appear in the modal if it is closed and re-opened. That's a departure from current behavior where the most recently updated issue would still appear if you close and reopen the modal with the same filters. My feeling is that clearing this on close would be the expected behavior, but that may be incorrect. Certainly it didn't make sense to keep all updated issues in recent state forever (or at least until the application is refreshed/re-scanned).

Some of these decisions/assumptions could benefit from being tested by UX folks.

@ssciolla ssciolla added bug javascript Pull requests that update Javascript code labels Feb 23, 2023
@ssciolla ssciolla marked this pull request as ready for review April 10, 2023 15:58
@ssciolla
Copy link
Contributor Author

If this is going to be considered, I'd consider reviewing #888 first. Making that change may reduce other error noise, especially for people with new local builds or setups.

@dmols dmols self-requested a review June 29, 2023 21:45
@dmols
Copy link
Contributor

dmols commented Jun 29, 2023

I see this being especially helpful for bigger courses; aiming to manually resolve several issues with the filtering involved tends to slow down the process a good bit.

Copy link
Contributor

@dmols dmols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. The UFixitModal works as intended and the issues can be manually resolved one after the other, without losing track of them. As mentioned, the resolved issues only go away upon closing the 'UFixitModal'. Clear to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recently updated state does not persist past the latest resolved or fixed issue?
2 participants