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

Add checks and warnings in SLR for non-empty directories #11088

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

subhramit
Copy link
Collaborator

@subhramit subhramit commented Mar 24, 2024

Fix koppor#600
Fix koppor#620

Description

When starting a new Systematic Literature Review (SLR), earlier, the users were allowed to create a new SLR (the Start survey button wasn't blocked) even if a non-empty directory was selected. There were no notes or warnings as well:

Screenshot 2024-03-24 165251

This would also sometimes cause the following exception to be thrown:

Screenshot 2024-03-24 170218

Through the changes made,

  1. A help text has been added as a note stating that the selected directory must be empty:
    Screenshot 2024-03-24 164357
  2. A warning (in red) is shown and the Start survey button is blocked if a non-empty directory is selected:
    Screenshot 2024-03-24 170717
    The Start survey button is now only enabled when the selected directory is empty (thus avoiding errors as well):
    Screenshot 2024-03-24 170742

Edit: For purposes of uniformity, we decided to stick to the global warning-message in Base.css instead of the red warning label depicted above:

image

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
    - [ ] Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@subhramit subhramit changed the title Added checks and warnings in SLR for non-empty directories Add checks and warnings in SLR for non-empty directories Mar 24, 2024
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Minor changes, otherwise: LGTM

Thank you for diving into the SLR topic. This is a very highly-ranked topic by me.

CHANGELOG.md Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinition.css Outdated Show resolved Hide resolved
@ThiloteE
Copy link
Member

To ease organizational workflows I have linked the pull-request to the issue with syntax as described in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

Linking a pull request to an issue using a keyword

You can link a pull request to an issue by using a supported keyword in the pull request's description or in a commit message. The pull request must be on the default branch.

  • close
  • closes
  • closed
  • fix
  • fixes
  • fixed
  • resolve
  • resolves
  • resolved

If you use a keyword to reference a pull request comment in another pull request, the pull requests will be linked. Merging the referencing pull request also closes the referenced pull request.

The syntax for closing keywords depends on whether the issue is in the same repository as the pull request.

Example:

  • Fix #xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix https://github.com/JabRef/jabref/issues/xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix https://github.com/Koppor/jabref/issues/xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix [#xyz](https://github.com/JabRef/jabref/issues/xyz) links pull-request to issue. Merging the PR will NOT close the Issue.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

See CSS comment

@@ -1308,6 +1308,10 @@ We want to have a look that matches our icons in the tool-bar */
-fx-text-fill: -jr-warn;
}

.warning-label {
Copy link
Member

Choose a reason for hiding this comment

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

Why couldn't you use the class .warning-message (instead of introducing a new class) with the color -jr-warn? (See line 76)

JabRef tries to have consistent UI colors.

Copy link
Collaborator Author

@subhramit subhramit Mar 25, 2024

Choose a reason for hiding this comment

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

Yes, actually I have night light on on my laptop, so couldn't see the orange warning too well. So I stuck to red.
Then when I realized night light was the reason, removed it. Sorry.

@subhramit
Copy link
Collaborator Author

To ease organizational workflows I have linked the pull-request to the issue with syntax as described in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

Linking a pull request to an issue using a keyword

You can link a pull request to an issue by using a supported keyword
in the pull request's description or in a commit message. The pull
request must be on the default branch.

  • close
  • closes
  • closed
  • fix
  • fixes
  • fixed
  • resolve
  • resolves
  • resolved

If you use a keyword to reference a pull request comment in another
pull request, the pull requests will be linked. Merging the referencing
pull request also closes the referenced pull request.
The syntax for closing keywords depends on whether the issue is in the same repository as the pull request.

Example:

  • Fix #xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix https://github.com/JabRef/jabref/issues/xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix https://github.com/Koppor/jabref/issues/xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix [#xyz](https://github.com/JabRef/jabref/issues/xyz) links pull-request to issue. Merging the PR will NOT close the Issue.

Thank you, edited!

@@ -2639,3 +2639,8 @@ Redownload\ missing\ files=Redownload missing files
Redownload\ missing\ files\ for\ current\ library?=Redownload missing files for current library?

File\ directory\ needs\ to\ be\ set\ or\ does\ not\ exist.\nPlease\ save\ your\ library\ and\ check\ the\ preferences.=File directory needs to be set or does not exist.\nPlease save your library and check the preferences.

Note\:\ The\ study\ directory\ should\ be\ empty.=Note: The study directory should be empty.
Copy link
Member

Choose a reason for hiding this comment

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

I think, the UI pattern of JabRef is to use colors to indicate "Note" and "Warning".

OK, we do have some strings with "Warning:" in front. Too much work to discuss this now. I'll just merge and we can do a follow-up for consistent UI later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

@koppor koppor enabled auto-merge March 25, 2024 13:27
@koppor koppor added this pull request to the merge queue Mar 25, 2024
Merged via the queue into JabRef:main with commit 9818ba8 Mar 25, 2024
20 checks passed
@subhramit subhramit deleted the slr-ui-changes branch March 25, 2024 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creation of a new SLR: Directory has to be empty [SLR] Help text on directory
3 participants