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

Close the tab that the cursor is on when right-clicking and selecting close #8193

Merged
merged 9 commits into from
Oct 29, 2021
Merged

Close the tab that the cursor is on when right-clicking and selecting close #8193

merged 9 commits into from
Oct 29, 2021

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Oct 26, 2021

Fixes #8180

The fix works for both Right-Click > Close and Right-Click > Close others
This fixes the whole context menu but now the PR title is misleading, I couldn't come up with a good one, if you have a better one, please edit it

  • 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 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.

@HoussemNasri
Copy link
Member Author

This unit test CompositeIdFetcherTest added recently in #8129 is failing

@Siedlerchr
Copy link
Member

Siedlerchr commented Oct 28, 2021

@HoussemNasri Oh thanks, probably forgotten to add it with the @FetcherTest annotation. Then it will be only run in the fetcher test workflow. Can you add it?

@HoussemNasri
Copy link
Member Author

My fork doesn't contain those changes, should I update it?

@Siedlerchr
Copy link
Member

You can directly fix that in this pr

@HoussemNasri
Copy link
Member Author

Can you elaborate more

@Siedlerchr
Copy link
Member

Add:
@FetcherTest
above

See also the other fetch test classes. The annotation is used to categorize and excklude the fetcher tests from the normal Test workflow as they will often fail due to rate limits when they are run everytime.

@HoussemNasri
Copy link
Member Author

Sorry for the misconception, I understand what the change should look like but my problem is mainly with Git/Github, my fork is outdated and I can't find CompositeIdFetcherTest , I don't know if updating my fork would create problems with the fix-8180 branch and this PR

@Siedlerchr
Copy link
Member

Ah okay, no, you should be able to do a git merge upstream/master into your branch. That should not create any conflicts (except for Changelog maybe)

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.

From my perspective the fixes look good, and I guess @koppor will be happy :)

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 28, 2021
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@calixtus calixtus merged commit 05d801e into JabRef:main Oct 29, 2021
@HoussemNasri HoussemNasri deleted the fix-8180 branch November 1, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context menu of a tab should be the one where the mouse cursor is on
3 participants