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 gray background text to authors field to assist newcomers #2147

Merged
merged 8 commits into from
Oct 12, 2016

Conversation

boceckts
Copy link
Contributor

@boceckts boceckts commented Oct 10, 2016

I added a gray background text to the "Author" and "Editor" field that appears when the fields don't contain any text. See koppor#61 for reference.

editorplaceholder

placeholderfocused

  • Interal quality assurance
  • Change in CHANGELOG.md described
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • If you changed the localization: Did you run gradle localizationUpdate?

Copy link
Contributor

@tschechlovdev tschechlovdev left a comment

Choose a reason for hiding this comment

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

Looks good from my side 👍


if (!this.hasFocus() && this.getText().isEmpty()) {
Font prev = g.getFont();
Color prevColor = g.getColor();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could rename some variables here.
g- > graphic
h -> height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tschechlovdev tschechlovdev added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Oct 10, 2016
@tschechlovdev tschechlovdev changed the title [WIP] Added gray background text to authors field to assist newcomers Added gray background text to authors field to assist newcomers Oct 10, 2016
super();
setupUndoRedo();
setupPasteListener();
this(null);
Copy link
Member

Choose a reason for hiding this comment

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

Why not "" instead of null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -151,7 +153,8 @@ private void setupPanel(JabRefFrame frame, BasePanel bPanel, boolean addKeyField
false);
defaultHeight = 0;
} else {
fieldEditor = new TextArea(field, null);
String prompt = (field.equals(FieldName.AUTHOR)) ? Localization.lang("Firstname Lastname and Firstname and Lastname and others") : "";
Copy link
Member

Choose a reason for hiding this comment

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

Wrong text

It should be Firstname Lastname and Firstname Lastname. and others must not be translated as it is a fixed bibtex term used to write et al. or something else as configured in the bibtex style.

The rendering of your example is
Lastname, F.; Firstname; Lastname & others
but it should be
Lastname, F.; Lastname, F. & others

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, as our "normalize to bibtex name format" formatter will produce "Last, First and Last, First and other", the text should be in this form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to Localization.lang("Firstname Lastname and Firstname Lastname") + " and others"

}

JTextAreaWithHighlighting(String text) {
super(text);
public JTextAreaWithHighlighting(String text, String title) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add javadoc comments at least describing title. Isn't title the textWhenNotFocused

Please rename textWhenNotFocused to placeholder to be in line with HTML5 --> http://www.w3schools.com/tags/att_input_placeholder.asp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@koppor
Copy link
Member

koppor commented Oct 11, 2016

I think, it's difficult to achieve the same effect as html5's placeholders:

There, when clicked in an empty field, the placeholder text remains in light gray. As soon as a key is pressed, the placeholder disappears.

Fun fact: If the focus is put away from JabRef, the text field is displayed as I wish - besides the missing cursor:
grabbed_20161011-042036

Should also be put for the field editor.

Other than that: LGTM, - I assume that before merge, all other required fields also have to get placeholders, This can possibly done by others more familiar with bib(la)tex if the developers agree on this concept.

@Siedlerchr
Copy link
Member

@koppor This can be achieved with a kind of Custom Component:
http://stackoverflow.com/questions/1738966/java-jtextfield-with-input-hint

@matthiasgeiger
Copy link
Member

@Siedlerchr thats the way it's currently implemented: Showing/Hiding the hint on FocusGained/FocusLost.

But of course, it is also possible to wait for a "KeyPressedEvent" to delete the placeholder.
However, I think using "isEmpty()" will no longer work, but a flag indicating whether there was an edit (i.e., a key has been pressed) will be necessary.

@koppor
Copy link
Member

koppor commented Oct 11, 2016

The solution to use WebLookAndFeel (http://stackoverflow.com/a/30782629/873282) also looks promising ( setInptPrompt), but the license of that library is GPL 😟

@koppor
Copy link
Member

koppor commented Oct 11, 2016

Can't just the simple solution using setText() and getText().equals(promptText) be used? http://stackoverflow.com/a/20200870/873282. Maybe with a combination of "selectAll()" when entering the field so that the users can directly start typing?

@boceckts
Copy link
Contributor Author

Using setText will save the text to the BibTeX source and  the text will apear in the fields of the maintable. I guess this is not desired.

On Oct 11, 2016, 09:32, at 09:32, Oliver Kopp notifications@github.com wrote:

Can't just the simple solution using setText() and
getText().equals(promptText) be used?
http://stackoverflow.com/a/20200870/873282

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#2147 (comment)

@matthiasgeiger
Copy link
Member

@boceckts Are you sure that a setText(...) will already trigger an edit of the BibEntry? I thought this is performed upon changing the active entry editor textfield...

@koppor
Copy link
Member

koppor commented Oct 11, 2016

Only when the text field gets focus. 😇

Could you try at the JavaFX branch? The solution http://stackoverflow.com/a/29946224/873282 looks promising.

`.text-field {
    -fx-prompt-text-fill: derive(-fx-control-inner-background,-30%);
}
.text-field:focused {
    -fx-prompt-text-fill: derive(-fx-control-inner-background,-30%);
}`

and add the 'promptText' entry in fxml file:

<TextField fx:id="XY_Id" promptText="First name">

No need to recode the whole tabbing, just a small test and if it works, we can decide to postpone a full implementation until javafx is our main UI technology.

@boceckts
Copy link
Contributor Author

I will give it a try. And yes I think it trigerres when the field loses it's focus.

On Oct 11, 2016, 09:40, at 09:40, Oliver Kopp notifications@github.com wrote:

Only when the text field gets focus. 😇

Could you try at the JavaFX branch? The solution
http://stackoverflow.com/a/29946224/873282 looks promising.

.text-field { -fx-prompt-text-fill: derive(-fx-control-inner-background,-30%); } .text-field:focused { -fx-prompt-text-fill: derive(-fx-control-inner-background,-30%); }

and add the 'promptText' entry in fxml file:

No need to recode the whole tabbing, just a small test and if it works,
we can decide to postpone a full implementation until javafx is our
main UI technology.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#2147 (comment)

@boceckts
Copy link
Contributor Author

I changed the behaviour so that the placeholder will always be displayed when the content of the text component is empty. This also means that the "search..." string will also be displayed when nothing is typed even when the search field is focused. I also renamed the classes to JTextAreaWithPlaceholder and JTextFieldWithPlaceholder. I also included the "Editor" field to display the same placeholder.

As @koppor mentioned, JavaFX TextField element has already a palceholder attribute which worked for me as you would expected. (Same as html5)

fieldEditor = new TextArea(field, null);
String prompt = "";
if (field.equals(FieldName.AUTHOR) || field.equals(FieldName.EDITOR)) {
prompt = Localization.lang("Firstname Lastname and Firstname Lastname") + " and others";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest removing the and from the localization as well as it is required that it stays and, therefore making it easier to translate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it accordingly.

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.

LGTM

@tobiasdiez tobiasdiez merged commit d962c1a into JabRef:master Oct 12, 2016
@boceckts boceckts deleted the grayBackgroundText branch October 12, 2016 12:32
@matthiasgeiger
Copy link
Member

This is not really working on master, is it?

@boceckts
Copy link
Contributor Author

@matthiasgeiger what do you mean, what exactly does not work? Is there an exception? I tested it on the current masters branch it works as expected...

@matthiasgeiger
Copy link
Member

Okay... you are right. Sorry, I tried the last build from https://builds.jabref.org/master which are currently outdated 😞

Siedlerchr added a commit that referenced this pull request Oct 13, 2016
* upstream/master: (24 commits)
  hotfix NPE in the main table (#2158)
  Enhance side pane toggle (#1605)
  Added gray background text to authors field to assist newcomers (#2147)
  Rewrite google scholar fetcher to new infrastructure (#2101)
  Found entries will be shown when dropping a pdf with xmp meta data (#2150)
  Japanese translation (#2155)
  Add shortcuts to context menu (#2131)
  [WIP] Doi resolution ignore (#2154)
  Fix DuplicationChecker and key generator (#2151)
  just close popup on first ESC
  keep entry in sight
  Fix isSharedDatabaseAlreadyPresent check.
  don't change entry after search if editing an entry (#2129)
  Change download URL to downloads.jabref.org (#2145)
  fix switching edited textfield in the entry editor with TAB (#2138)
  Update me.champeau.gradle.jmh' version from 0.3.0 to 0.3.1
  Update install4j plugin from 6.1.2 to 6.1.3
  Remove gradle plugin com.github.youribonnaffe.gradle.format as it is deprecated and the config uses a non-existing file
  Update gradle from 3.0 to 3.1
  Fix changing the font size not working when importing preferences (#2069)
  ...

# Conflicts:
#	src/main/java/net/sf/jabref/gui/externalfiles/DroppedFileHandler.java
#	src/main/java/net/sf/jabref/gui/importer/fetcher/GoogleScholarFetcher.java
@boceckts boceckts mentioned this pull request Oct 16, 2016
4 tasks
zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
…ef#2147)

* Added gray background text to assist newcomers

* Display placeholder even when focused

* Change placeholder string and include editor field

* Update changelog

* Fix localization

* Refactor localization
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.

7 participants