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 various Dark theme issues #6368

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Fix various Dark theme issues #6368

merged 2 commits into from
Apr 28, 2020

Conversation

MootezSaaD
Copy link
Contributor

Related issues: #5522 , #5699

  • Drag'n'drop text

image

  • Dialogs
    image

image

image

The issue is that I don't find the dialogs graphics (e.g. the confirmation image) adequate, so maybe create custom ones for the dark theme (or remove them)?

  • Scroll pane
    The scroll pane is quite visible (at least on my side) so I don't know if that issue has been considered as resolved or not?

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.

tobiasdiez
tobiasdiez requested changes Apr 28, 2020
View reviewed changes
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.

Thanks a lot, this looks very nice.

The issue is that I don't find the dialogs graphics (e.g. the confirmation image) adequate, so maybe create custom ones for the dark theme (or remove them)?

Good point. I guess, the best solution would be to replace these icons by font-based icons, for example https://materialdesignicons.com/icon/information-outline. This then also gives a more consistent look with the other icons we already use in the toolbar or menu. But this can wait for another PR.

The scroll pane is quite visible (at least on my side) so I don't know if that issue has been considered as resolved or not?

I think it's still somewhat hard to recognize. Could you try to increase the contrast a bit?

src/main/java/org/jabref/gui/JabRefDialogService.java Outdated
@@ -126,6 +127,7 @@ public static String shortenDialogMessage(String dialogMessage) {
choiceDialog.setHeaderText(title);
choiceDialog.setTitle(title);
choiceDialog.setContentText(content);
Globals.getThemeLoader().installCss(choiceDialog.getDialogPane().getScene(), Globals.prefs);
Copy link
Member

Choose a reason for hiding this comment

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

We are trying to remove the Globals class (in the long term). To facilitate this, could you please extract the themeLoader and preferences, and initialize them by passing the global objects as dependencies through the constructor (i.e. move Globals from this class to the caller)

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 have refactored the code but I m not sure if I've done it correctly

Siedlerchr
Siedlerchr reviewed Apr 28, 2020
View reviewed changes
src/main/java/org/jabref/gui/JabRefDialogService.java Outdated
@@ -148,12 +150,14 @@ public static String shortenDialogMessage(String dialogMessage) {
@Override
public void showInformationDialogAndWait(String title, String content) {
FXDialog alert = createDialog(AlertType.INFORMATION, title, content);
Globals.getThemeLoader().installCss(alert.getDialogPane().getScene(), Globals.prefs);
Copy link
Member

Choose a reason for hiding this comment

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

Does it work, if you move this line inside the createDialog? Then you could reduce some code duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! didn't really pay attention.

@MootezSaaD
Copy link
Contributor Author

Changes regarding the scroll bar:

image

On hover:
image

@tobiasdiez
Copy link
Member

Perfect. Thank you again!

@tobiasdiez tobiasdiez merged commit cbf7796 into JabRef:master Apr 28, 2020
@calixtus calixtus mentioned this pull request Apr 28, 2020
Closed
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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr left review comments

@tobiasdiez tobiasdiez tobiasdiez requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@MootezSaaD @tobiasdiez @Siedlerchr