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.

Sign up for GitHub

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

Jump to bottom

Fix spacing in Cleanup dialog #10081 #10085

Merged
merged 10 commits into from
Jul 15, 2023
Merged

Fix spacing in Cleanup dialog #10081 #10085

merged 10 commits into from
Jul 15, 2023

Conversation

pcabaniss
Copy link
Contributor

@pcabaniss pcabaniss commented Jul 15, 2023 •
edited by Siedlerchr
Loading

Fixes #10081 , Updated fxml file in Scenebuilder. Fixed spacing and indentation.

Screenshot 2023-07-15 at 2 55 29 AM

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.

calixtus
calixtus reviewed Jul 15, 2023
View reviewed changes
.vscode/launch.json Outdated
Comment on lines 1 to 15
{
// Use IntelliSense to learn about possible attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [

{
"type": "java",
"name": "Current File",
"request": "launch",
"mainClass": "${file}"
}
]
}
Copy link
Member

Choose a reason for hiding this comment

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

This is probably an unwanted artifact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this is the first contribution I've ever done. Is this bad practice?

Copy link
Member

Choose a reason for hiding this comment

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

No problem, we are here to help! 😍

@calixtus
Copy link
Member

Great job for your first contribution!
Just made some small whitespace changes.
Just as a suggestion: If you want to create changes, first create a new branch instead of directly commiting on your main branch. This way you can always keep multiple branches of you are working on multiple projects and you always have your main branch in sync with the official repo.

@pcabaniss
Copy link
Contributor Author

Thanks! I'll remember that for next time. If there's any other criticism, please let me know.😁

calixtus
calixtus previously approved these changes Jul 15, 2023
View reviewed changes
Copy link
Member

@calixtus calixtus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good to me. LGTM 👍. The changes need a second review and thus this PR awaits a second review.

@calixtus calixtus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers ui cleanup-ops labels Jul 15, 2023
@calixtus
Copy link
Member

Failing unit test is not related to your changes. Fixed by #10086

Siedlerchr
Siedlerchr previously approved these changes Jul 15, 2023
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! A small change with huge impact!
Can you please also add an entry to the Changelog.md ?

@pcabaniss
Copy link
Contributor Author

I changed it and committed, do I need to make another PR? Or does it automatically merge with this one?

@Siedlerchr
Copy link
Member

Just commit and push your commits, the PR will be updated automatically.
When merging, we "squash" all commits into one.

@pcabaniss
Copy link
Contributor Author

Done. Thank you both for your help and patience.

@calixtus
Copy link
Member

After committing the changes, you still have to push the commits to the remote (upstream) repo. 'git push'

@pcabaniss pcabaniss dismissed stale reviews from calixtus and Siedlerchr via 2aff909 July 15, 2023 10:03
@Siedlerchr
Copy link
Member

PS: We recommend Intellij for setting up your workspace https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/

@Siedlerchr Siedlerchr merged commit 76be2a7 into JabRef:main Jul 15, 2023
@Siedlerchr
Copy link
Member

Thanks again for your contribution! Also make sure to create a new branch the next time, makes it easier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@calixtus calixtus calixtus left review comments

Assignees
No one assigned
Labels
cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix spacing in Cleanup dialog
3 participants
@pcabaniss @calixtus @Siedlerchr