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 wrong button order (Apply and Cancel) in ManageProtectedTermsDialog. #6358

Merged
merged 2 commits into from
Apr 27, 2020
Merged

Fix wrong button order (Apply and Cancel) in ManageProtectedTermsDialog. #6358

merged 2 commits into from
Apr 27, 2020

Conversation

dextep
Copy link
Contributor

@dextep dextep commented Apr 26, 2020 •
edited
Loading

Hi, I noticed that 'manage protected terms files' dialog button order was wrong - 'Apply' was on right side.

image

  • Change in CHANGELOG.md described (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 created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

koppor
koppor reviewed Apr 26, 2020
View reviewed changes
src/main/java/org/jabref/gui/protectedterms/ManageProtectedTermsDialog.java
@@ -3,12 +3,9 @@
import javax.inject.Inject;

import javafx.fxml.FXML;
import javafx.scene.control.ButtonType;
Copy link
Member

Choose a reason for hiding this comment

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

Please check your IDE setup. It seems that you did not import the JabRef code style. --> https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. Thanks @koppor!

koppor
koppor reviewed Apr 26, 2020
View reviewed changes
src/main/java/org/jabref/gui/protectedterms/ManageProtectedTermsDialog.fxml Outdated
@@ -31,6 +31,6 @@
</HBox>
</VBox>
</content>
<ButtonType fx:id="applyButton" buttonData="OK_DONE" text="%Apply" />
Copy link
Member

Choose a reason for hiding this comment

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

The order of buttons is OS depdenend. @tobiasdiez Does one have apply a special CSS class here? It does not seem to be applied correclty here (refs https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/ButtonType.html#APPLY)

Copy link
Contributor Author

@dextep dextep Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have checked another dialog class CustomizeEntryTypeDialog and i wanted to keep the pattern, but right now i checked it again and this id seems to be unused. In this case OSX have the same button order.

image

Copy link
Member

Choose a reason for hiding this comment

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

The order is displayed here: https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/ButtonBar.html

ButtonType = APPLY is indeed wrong, as this is for a button that saves the current state but leaves the dialog open. OK_DONE is correct. I would maybe change the display text to %Save to make it clearer and prevent confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Yes the id can be safely deleted. It's only needed if the code uses lookupButton

@dextep
Fix code style.
51e2bda 
Change button text 'Apply' to 'Save'.
@Siedlerchr Siedlerchr merged commit 5292a70 into JabRef:master Apr 27, 2020
@Siedlerchr
Copy link
Member

Thank you very much for your contribution!

Siedlerchr added a commit that referenced this pull request Apr 30, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixExporterSelect…
6dea49e 
…ionCaseInsensitive

* upstream/master:
  New Crowdin translations (#6375)
  Squashed 'src/main/resources/csl-styles/' changes from 143464e..906cd6d
  Fixes #6357: File directory (#6377)
  Disable the generate button if the ID field is empty (#6371)
  Fix Preferences style value too long (#6372)
  Fix various Dark theme issues (#6368)
  Correct label name in dependabot
  Bump java-diff-utils from 4.5 to 4.7 (#6365)
  Try with info.plist.template also (#6366)
  Fix wrong button order (Apply and Cancel) in ManageProtectedTermsDialog. (#6358)
  Bump flexmark-ext-gfm-strikethrough from 0.61.6 to 0.61.20 (#6361)
  Bump checkstyle from 8.31 to 8.32 (#6360)
  Bump flexmark-ext-gfm-tasklist from 0.61.16 to 0.61.20 (#6364)
  Bump flexmark from 0.61.16 to 0.61.20 (#6359)
  Bump org.beryx.jlink from 2.17.7 to 2.17.8 (#6362)
  Bump classgraph from 4.8.71 to 4.8.77 (#6363)
  Change blue and red colors in Merge entries dialog in Dark theme (#6356)
  Add Info plist to mac resources (#5986)
  Backward compatibility for 4.3.1 (#6296)
Siedlerchr added a commit that referenced this pull request May 2, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into allowReordering
2a48588 
* upstream/master: (166 commits)
  New Crowdin translations (#6382)
  Update code-howtos.md (#6393)
  Fix jstyle was invalid with default section at the start (#6386)
  Correcting file name for groups.uml (#6373)
  Fix underscore character being omitted from file name in Recent Libraries list (#6389)
  Rework journal abbreviation caching (#6304)
  Fix selecting custom export for copy to clipboard with uppercase file ext (#6290)
  New Crowdin translations (#6375)
  Squashed 'src/main/resources/csl-styles/' changes from 143464e..906cd6d
  Fixes #6357: File directory (#6377)
  Disable the generate button if the ID field is empty (#6371)
  Fix Preferences style value too long (#6372)
  Fix various Dark theme issues (#6368)
  Correct label name in dependabot
  Bump java-diff-utils from 4.5 to 4.7 (#6365)
  Try with info.plist.template also (#6366)
  Fix wrong button order (Apply and Cancel) in ManageProtectedTermsDialog. (#6358)
  Bump flexmark-ext-gfm-strikethrough from 0.61.6 to 0.61.20 (#6361)
  Bump checkstyle from 8.31 to 8.32 (#6360)
  Bump flexmark-ext-gfm-tasklist from 0.61.16 to 0.61.20 (#6364)
  ...
koppor pushed a commit that referenced this pull request Jan 6, 2023
Squashed 'buildres/csl/csl-styles/' changes from 43566f2..168aa3c021
5984ed8 
168aa3c021 Update molecular-ecology.csl (#6363)
15f1c9907b Update indian-journal-of-orthopaedics.csl (#6358)
a18a7475cb Update frontiers.csl (#6360)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 168aa3c0214582195cb5280fa4c7146f55ce187e
koppor pushed a commit that referenced this pull request Jan 15, 2023
Squashed 'buildres/csl/csl-styles/' changes from 43566f2..50eea55b2c
0a6468f 
50eea55b2c Gaceta Sanitaria: adjust et-al to align w guide
7b26635231 Create journal-of-prosthodontics.csl (#6320)
fcffad7b5d Update and rename world-politcs.csl to world-politics.csl (#6346)
e89fb04a21 Update environmental-conservation.csl (#6312)
168aa3c021 Update molecular-ecology.csl (#6363)
15f1c9907b Update indian-journal-of-orthopaedics.csl (#6358)
a18a7475cb Update frontiers.csl (#6360)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 50eea55b2c512833fec6c8b810f7e73b53b2f611
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koppor koppor koppor left review comments

@tobiasdiez tobiasdiez tobiasdiez approved these changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dextep @Siedlerchr @koppor @tobiasdiez