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

Change export to save #7518

Merged
merged 4 commits into from
Apr 5, 2021
Merged

Change export to save #7518

merged 4 commits into from
Apr 5, 2021

Conversation

yinpeiqi
Copy link
Contributor

@yinpeiqi yinpeiqi commented Mar 12, 2021 •
edited by Siedlerchr
Loading

fix for 7517, change Export to Save
Fixes #7517

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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.

@mlep
Copy link
Contributor

mlep commented Mar 12, 2021

This is one of the fastest Issue --> PR I have seen!
Looking at your changes, I think the section title "Export sort order" needs to be modified too (and maybe twice: once for the preferences window, and once for the library window).

@yinpeiqi
Copy link
Contributor Author

Okay I changed it too.

@Siedlerchr
Copy link
Member

You probably need to adjust the localization in the l10n properties file https://devdocs.jabref.org/getting-into-the-code/code-howtos#using-localization-correctly

@yinpeiqi
Copy link
Contributor Author

Only delete the unused part of l10n is enough, am I right?

@Siedlerchr Siedlerchr closed this Mar 13, 2021
@Siedlerchr Siedlerchr reopened this Mar 13, 2021
@Siedlerchr Siedlerchr changed the title Fix for issue 7517 Change export to save Mar 14, 2021
@calixtus
Copy link
Member

Please check your github configuration. The checks all stall and without checks it's way harder for us to test changes made to the codebase!

@koppor
Copy link
Member

koppor commented Mar 29, 2021

Please keep the distinction of "Export" and "Save". Please change to "neutral" wording to enable use of a common FX control:

grafik

(See also #7517 (comment))

@Siedlerchr
Copy link
Member

@yinpeiqi It would be really nice if you could implement the changes as outlined in the screenshots

@yinpeiqi
Copy link
Contributor Author

yinpeiqi commented Apr 4, 2021

Is this change ok?

calixtus
calixtus reviewed Apr 4, 2021
View reviewed changes
src/main/java/org/jabref/gui/commonfxcontrols/SaveOrderConfigPanel.java Outdated
Comment on lines 73 to 77
public void changeExportDescriptionToSave() {
exportInOriginalOrder.setText(Localization.lang("Save entries in their original order"));
exportInSpecifiedOrder.setText(Localization.lang("Save entries ordered as specified"));
exportInTableOrder.setText(Localization.lang("Save in current table sort order"));
exportInOriginalOrder.setText(Localization.lang("Keep original order"));
exportInSpecifiedOrder.setText(Localization.lang("Use specified order"));
exportInTableOrder.setText(Localization.lang("Use current table sort order"));
}
Copy link
Member

Choose a reason for hiding this comment

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

This method and the call to can be removed entirely.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Otherwise it's good from my side,tested it locally

@yinpeiqi
Copy link
Contributor Author

yinpeiqi commented Apr 5, 2021

Okay, I remove it.

@calixtus
Copy link
Member

calixtus commented Apr 5, 2021

There is still a unused import, breaking a check style rule.

@yinpeiqi
Copy link
Contributor Author

yinpeiqi commented Apr 5, 2021

Well, this time I remove it.

calixtus
calixtus approved these changes Apr 5, 2021
View reviewed changes
Copy link
Member

@calixtus calixtus 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 to me! Thanks.

@yinpeiqi
Copy link
Contributor Author

yinpeiqi commented Apr 5, 2021

You are welcome.

@Siedlerchr Siedlerchr merged commit 6837164 into JabRef:main Apr 5, 2021
@Siedlerchr
Copy link
Member

Thanks again for your contribution!

Siedlerchr added a commit that referenced this pull request Apr 11, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into writebackupToTemp
5b2b64c 
* upstream/main:
  Main instead of master
  Custom DOI base address fix (#7569)
  Change export to save (#7518)
  Bump unoloader from 7.1.1 to 7.1.2 (#7609)
  Bump org.beryx.jlink from 2.23.5 to 2.23.6 (#7610)
  Bump com.adarshr.test-logger from 2.1.1 to 3.0.0 (#7611)
  Bump libreoffice from 7.1.1 to 7.1.2 (#7612)
  Squashed 'buildres/csl/csl-styles/' changes from e1acabe..bfa3b6d (#7603)
  Rename master to main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixtus calixtus calixtus 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.

UI: "Export order" --> "Save order"
5 participants
@yinpeiqi @mlep @Siedlerchr @calixtus @koppor