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

Pressing escape during search clears search field & selects first entry #3747

Merged
merged 8 commits into from
Feb 19, 2018
Merged

Pressing escape during search clears search field & selects first entry #3747

merged 8 commits into from
Feb 19, 2018

Conversation

mohamean
Copy link
Contributor

@mohamean mohamean commented Feb 19, 2018

Fixes: koppor#293

Allows the user to clear the search field by pressing the escape key. As requested as well, the first entry in the table is automatically selected afterwards, if such an entry exists.


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@mohamean mohamean changed the title Pressing escape Pressing escape during search clears search field & selects first entry Feb 19, 2018
@Siedlerchr
Copy link
Member

Thank you very much for your contribution! Codewise looks good!
But as we are in the migration to JavaFX, would you mind trying to implement this on top of the maintable-beta branch/javafxgloba everything?
(I think you better open a new PR for that javafx part then)

CHANGELOG.md Outdated
@@ -34,6 +34,7 @@ The new default removes the linked file from the entry instead of deleting the f
- The group editing window can now also be called by double-clicking the group to be edited. [koppor#277](https://github.com/koppor/jabref/issues/277)
- The magnifier icon at the search shows the [search mode](https://help.jabref.org/en/Search#search-modes) again. [#3535](https://github.com/JabRef/jabref/issues/3535)
- We added a new cleanup operation that replaces ligatures with their expanded form. [#3613](https://github.com/JabRef/jabref/issues/3613)
- Pressing ESC while searching will clear the search field and select the first entry, if available, in the table. [koppor#293](https://github.com/koppor/jabref/issues/293)
Copy link
Member

Choose a reason for hiding this comment

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

Just one minor thing: you should wrap keycodes in kbd tags, like <kbd>ESC</kbd>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh so that's what was meant by wrapping keys in the changelog. Didn't know it actually means keycodes. My bad.

Copy link
Member

Choose a reason for hiding this comment

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

While you are on it, could you do that here? ESC?

@mohamean
Copy link
Contributor Author

Will do, no worries. Are both branches maintable-beta & javafxGlobalEverything equivalent or should I choose one over the other?

@tobiasdiez
Copy link
Member

Please base it at javafxGlobalEverything, there were some major changes to the menu and toolbar as @Siedlerchr said. Thanks!

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 19, 2018
@koppor koppor added the PE1718 label Feb 19, 2018
/**
* Clears the search bar and select first entry, if available
*
* @author Aly Mohamed
Copy link
Member

Choose a reason for hiding this comment

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

Please do not add @author tags even if the exercise sheet requested it.

Copy link
Contributor Author

@mohamean mohamean Feb 19, 2018

Choose a reason for hiding this comment

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

Should I then push a new commit with the author tag removed or should I do that in the new pull request which would be based on the branch javafxGlobalEverything?

Copy link
Member

Choose a reason for hiding this comment

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

Just remove it on this PR. Then, this PR is ready for merge.

Please create a new branch based on javafxGlobalEverything. I think, the implementation will differ much, so you can start from scratch? 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, no problem 😃 I'm actually enjoying this myself and am learning a lot about developing larger-scale open-source projects so starting from scratch sounds fun 😄.

CHANGELOG.md Outdated
@@ -34,6 +34,7 @@ The new default removes the linked file from the entry instead of deleting the f
- The group editing window can now also be called by double-clicking the group to be edited. [koppor#277](https://github.com/koppor/jabref/issues/277)
- The magnifier icon at the search shows the [search mode](https://help.jabref.org/en/Search#search-modes) again. [#3535](https://github.com/JabRef/jabref/issues/3535)
- We added a new cleanup operation that replaces ligatures with their expanded form. [#3613](https://github.com/JabRef/jabref/issues/3613)
- Pressing <kbd>ESC<kbd> while searching will clear the search field and select the first entry, if available, in the table. [koppor#293](https://github.com/koppor/jabref/issues/293)
Copy link
Member

Choose a reason for hiding this comment

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

The / got lost at this quick fix. Note that the closing XML element is </kbd>.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 19, 2018
@koppor koppor merged commit 5ab54f1 into JabRef:master Feb 19, 2018
@mohamean mohamean deleted the fix-for-issue-674 branch February 20, 2018 13:05
@tobiasdiez
Copy link
Member

@mohamean I just merged the globalEveryThing branch into "maintable-beta" thus you can now use the latter as the base.

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.

None yet

4 participants