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

Added a "Clear text" button in right click menu within the text boxes. #3475

Merged
merged 5 commits into from
Dec 12, 2017

Conversation

weidafan
Copy link
Contributor

@weidafan weidafan commented Dec 1, 2017

Adresses koppor#198

Revised pull request from Monday 11/27. #3465

It was requested to have a new button within the right click menu of text boxes when creating or editing library entries. See this link for description
We created a new class that extended MenuItem to create the instance of the ClearField.
We added the instance into the menuItem array so that it would be included in the right click menu.

There is a possibility to make each text box a clearable text box, with a red "x" in the corner to clear the text box even quicker. We could not target where to change the type of text box, so that feature still remains.


  • 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?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@Siedlerchr
Copy link
Member

For future contributions, please always update the existing PR (except you messed up) . That makes it easier to follow and review your code. On merge all commits will be squashed into one

@weidafan
Copy link
Contributor Author

weidafan commented Dec 1, 2017

I am sorry. I am a beginner. This is my first pull request and I did mess up my whole JabRef program. I rebuilded the whole program. Thank you for your advise.

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

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

I have a few very minor comments that should be addressed, but apart from that the PR looks good.

Thanks for your contribution!

CHANGELOG.md Outdated
@@ -34,6 +34,8 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We added an option to mass append to fields via the Quality -> set/clear/append/rename fields dialog. [#2721](https://github.com/JabRef/jabref/issues/2721)
- We added a check on startup to ensure JabRef is run with an adequate Java version. [3310](https://github.com/JabRef/jabref/issues/3310)
- In the preference, all installed java Look and Feels are now listed and selectable
- We added the clear text button into the right-click menu of the text field. When adding a new entry into a library, the text boxes are now clearable by clicking this button. Issue 198 in "Beginner" section of "koppor"'s repository. ( https://github.com/jabref/issues/198 )
Copy link
Member

Choose a reason for hiding this comment

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

Please format the link to the issue in the same way as for the other changelog entries.

super(Localization.lang("Clear text"));
setOnAction(event -> opener.setText(""));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please end code files with a new line

@@ -84,4 +84,4 @@
return menuItems;
};
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

As above, please do not remove the new line here.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I still have a few remarks before this can be merged.

CHANGELOG.md Outdated
@@ -34,6 +34,8 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We added an option to mass append to fields via the Quality -> set/clear/append/rename fields dialog. [#2721](https://github.com/JabRef/jabref/issues/2721)
- We added a check on startup to ensure JabRef is run with an adequate Java version. [3310](https://github.com/JabRef/jabref/issues/3310)
- In the preference, all installed java Look and Feels are now listed and selectable
- We added the clear text button into the right-click menu of the text field. When adding a new entry into a library, the text boxes are now clearable by clicking this button. Issue 198 in "Beginner" section of "koppor"'s repository. [#198](https://github.com/jabref/issues/198)
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 description

We added a clear option to the right-click menu of the text field in the entry editor.
is already sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Moreover, please reference to the issue in the form koppor#198 and link the correct issue.

super(Localization.lang("Clear text"));
setOnAction(event -> opener.setText(""));
}

Copy link
Member

Choose a reason for hiding this comment

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

Well, the new line character is now at the wrong place 😸. It should be after the last brace and not before (the red "no newline at end of file" marker here at github should disappear. Your IDE probably has an option for adding the newline automatically.

class ClearField extends MenuItem {

public ClearField(TextArea opener) {
super(Localization.lang("Clear text"));
Copy link
Member

Choose a reason for hiding this comment

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

You can also just reuse the text "Clear" here so that our poor translators don't have to work too much.

@@ -199,6 +199,7 @@ Class_name=Klassenavn
Clear=Ryd

Clear_fields=Ryd_felter
Clear_text=
Copy link
Member

Choose a reason for hiding this comment

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

You still need to delete these now unused translation strings.

@tobiasdiez tobiasdiez merged commit bd42bb4 into JabRef:master Dec 12, 2017
@tobiasdiez
Copy link
Member

Thanks again for your contribution!

@brandonwalker08
Copy link

@tobiasdiez @lenhard @Siedlerchr Thank you all for your help. Our first time contributing to an open source project was a good experience thanks to your feedback.

Siedlerchr added a commit that referenced this pull request Dec 13, 2017
* upstream/master: (108 commits)
  Fetcher for IACR eprints (#3473)
  Update internal state of DatabaseChangeMonitor when external changes … (#3503)
  Fixes #3505: Another try to fix the NPE in the search bar (#3512)
  Replace ' with ' so that our HTML preview can handle it correctly
  Added a "Clear text" button in right click menu within the text boxes. (#3475)
  Add reset to English language after a test
  New translations JabRef_en.properties (German)
  Remove ampersand in non-menu localizations
  New translations JabRef_en.properties (German)
  New translations Menu_en.properties (German)
  New translations Menu_en.properties (German)
  New translations JabRef_en.properties (Vietnamese)
  New translations JabRef_en.properties (Italian)
  New translations Menu_en.properties (Italian)
  New translations JabRef_en.properties (Indonesian)
  New translations Menu_en.properties (Indonesian)
  New translations JabRef_en.properties (Greek)
  New translations Menu_en.properties (Greek)
  New translations Menu_en.properties (Japanese)
  New translations JabRef_en.properties (German)
  ...

# Conflicts:
#	build.gradle
@LinusDietz
Copy link
Member

@weidafan We are about to release a new Version of JabRef and would like to attribute you with your full name. Can you tell us your name, so we can include it?

@weidafan
Copy link
Contributor Author

weidafan commented Dec 22, 2017 via email

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.

6 participants