Skip to content

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

Merged
Siedlerchr merged 2 commits into
JabRef:masterfrom
dextep:wrong-button-order
Apr 27, 2020
Merged

Fix wrong button order (Apply and Cancel) in ManageProtectedTermsDialog.#6358
Siedlerchr merged 2 commits into
JabRef:masterfrom
dextep:wrong-button-order

Conversation

@dextep

@dextep dextep commented Apr 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread src/main/java/org/jabref/gui/protectedterms/ManageProtectedTermsDialog.java
import javax.inject.Inject;

import javafx.fxml.FXML;
import javafx.scene.control.ButtonType;

@koppor koppor Apr 26, 2020

Copy link
Copy Markdown
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

@dextep dextep Apr 26, 2020

Copy link
Copy Markdown
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
Comment thread src/main/java/org/jabref/gui/protectedterms/ManageProtectedTermsDialog.fxml Outdated
</HBox>
</VBox>
</content>
<ButtonType fx:id="applyButton" buttonData="OK_DONE" text="%Apply" />

@koppor koppor Apr 26, 2020

Copy link
Copy Markdown
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)

@dextep dextep Apr 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@tobiasdiez tobiasdiez Apr 26, 2020

Copy link
Copy Markdown
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.

@tobiasdiez tobiasdiez Apr 26, 2020

Copy link
Copy Markdown
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

Siedlerchr commented Apr 27, 2020

Copy link
Copy Markdown
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

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@tobiasdiez tobiasdiez tobiasdiez 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