-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Consistent FX color scheme for JabRef #3839
Conversation
# Conflicts: # src/main/java/org/jabref/gui/maintable/MainTable.css
# Conflicts: # src/main/java/org/jabref/gui/maintable/MainTable.css
I played around with the UI a little and I like it. I like the fact that the menu and tool bars are a little less color-intense than in the current maintable-beta. You have my +1 for continuing with this line of work. |
In the main toolbar, I changed the "New File" icon because it was an outlined version while Open and Save is a filled version. For the Open icon I would like to have the "folder-open", but that is currently not included in our version. I changed the Add Entry icon to a filled version as well, since it better fits the trash. Unfortunately, there is no filled version of Copy and Paste and the positively filled version of GitHub looks just awful. I changed the Up, Down, and Close icon for the sidepanel and the entry editor to versions that look visually more similar in size. Especially, the up and down was too small. After correspondence with a friend, I changed the default list item before the groups into something not so heavy. The small triangle with its vertical side on the left gives additionally a better impression of the indentation which is the key here. The Add Group icon was changed into a heavier circled version. It now matches the style of the close button of the sidepanel and entry editor. All icons of "sub toolbars" in the entry editor and the side panel have now an equal size of 16px. The preferred height of the side-panel header and the header of the tab-pane was adjusted so that they match and there is no jump between main-table and side-panel anymore. There might be a better way to do this. Finally, the color of the overflow buttons for toolbar and tab-pane was adjusted.
Slightly reducing this color still looks very nice and it gives a better impression when text is marked.
Additionally, an alternative version for Vim was added and slight improvements on other icons.
I made further improvements to the custom icons and they look at least acceptable now if put on the toolbar: For more information on this view issue #3868. I pulled the latest changes from maintable-beta. I'm giving this free for review now. Note that there are still a lot of commented lines in the code to easily see what I have adjusted. Once we agree on something, I'll clean everything up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for your work. I really like the central place for colors, makes it way easier to read and understand the rest of the css file. I have a few remarks, which are mostly "please reactivate this code you commented-out". I think, you can cleanup the code and then we can merge. I'll try to design a dark theme but that may take some while.
@@ -60,6 +61,8 @@ | |||
} | |||
|
|||
private static InputStream getMaterialDesignIconsStream() { | |||
// TODO: The next line loads the additional JabRef-Icons. Most certainly wrong at this place |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is font-loading handled in the jensd.fx.glyphs
library? I would follow a similar strategy as them...Another idea is to create a new static method and call it during application initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this and used the same approach we (and Material Design Icons as well) use. It's now loaded the static initializer of the IconTheme
class itself.
APPLICATION_WINEDT(IconTheme.getImage("winedt")), | ||
FILE_OPENOFFICE(JabRefMaterialDesignIcon.OPEN_OFFICE), | ||
APPLICATION_EMACS(JabRefMaterialDesignIcon.EMACS), | ||
APPLICATION_LYX(JabRefMaterialDesignIcon.LYX), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other places that still use IconTheme.getImage
? If not this method can be removed as well as InternalFileIcon
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored this out. I initially left it because maybe someone still needs a pixel icon in future. But since it is really easy now to add SVG icons if we indeed don't find anything appropriate in the existing ones, this can be removed.
-jr-base: #f0f0f0; | ||
|
||
-jr-white: #ffffff; | ||
-jr-gray-00: #f5f5f5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for the 00
suffix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what happens if you still need a lighter gray than -jr-gray-0
:) I fixed it.
-jr-tooltip-bg: -jr-info; | ||
-jr-tooltip-fg: -jr-black; | ||
|
||
/* Consistent size for headers of tab-pane and side-panels*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this down to the end, since right now it appears in the middle of color definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
.menu-bar > .container > .menu-button > .label { | ||
-fx-padding: 0.41777em 0.41777em 0.41777em 0.41777em; /* 5 5 5 5 */ | ||
} | ||
/*.menu-bar > .container > .menu-button > .label {*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for uncommenting these menu bar customization? If I remember correctly, they make the menu items a bit bigger to align with the MaterialDesing specifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look good. Even the default padding is larger than it is in other applications, but it looks OK. Compare these two images that show additionally the header of the mail program. First, as I have it now
And here with the additional padding
The menu items look lost in the menu bar. The spacing is far too large. I really would like to keep the default setting.
Btw, in the colorful version, we have now on maintable-beta
, this kind of made sense because it tried to mimic the look of today's web-pages with their very large top menus. If you decide to work on a different scheme, the large padding might be an option again. But in this scheme, it doesn't work and I'd like to keep it simple and unsurprising for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still like the second screenshot more and find it more touch-friendly. But, well, we just have different tastes and I definitely understand your viewpoint.
Probably every dev should create an own theme and we then vote which should be the default 😸
-fx-table-cell-border-color: transparent; /* hide grid lines */ | ||
} | ||
|
||
.tree-table-cell { | ||
-fx-font-size: 105%; | ||
/*-fx-font-size: 105%;*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably personal preference, but I prefer the bigger font size (and smaller for hits).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm against this. Although the font is only one pixel larger on my system and the anti-aliasing is also OK, it introduces a very notable font-difference to the main-table where there should be consistency. I would accept if this is wanted by most devs, but from my point of view, it is not OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, convinced. The base font size should be the same. But I still think the hit-number can be smaller because this is kind of unimportant info.
By the way, did you change the padding of the tree node items? They have a bit tighter feeling than on the current master/maintable-beta, which probably results not from the difference of font-sizes.
} | ||
|
||
.disclosureNodeColumn { | ||
-fx-alignment: top-right; | ||
} | ||
|
||
.tree-table-row-cell:dragOver-bottom { | ||
-fx-border-color: #eea82f; | ||
-fx-border-color: -jr-yellow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should reuse the same active/selected colors as for the main table, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I introduced a color for it in Main.css
and set it to our orange. This looks decent and the indicator is visible when you drag it over a marked entry.
-fx-background-color: white; | ||
} | ||
/*.tree-table-row-cell:selected > .tree-table-cell > .tree-disclosure-node > .arrow {*/ | ||
/*-fx-background-color: -jr-white;*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your screenshot, the arrow is never visible accept on hover because it is also white...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my tests, the commented section is the color of the small arrow, when the group is selected. This setting is not required anymore as far as I understand it because the arrow has not the same color as the icons and our markup color is bright. It looks currently like this
So for me, the arrow is always visible and looks OK even when the entry is selected.
} | ||
|
||
.table-view .column-header .glyph-icon { | ||
-fx-alignment: baseline-center; | ||
-fx-text-fill: -fx-unimportant; | ||
-fx-fill: -fx-unimportant; | ||
-fx-text-fill: -jr-icon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use fx-mid-text instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks pretty decent to me with the icon color and since the table header entry can be pressed to invoke the sorting action, it made sense to me to indicate this. But this might also a very personal preference.
.table-row-cell:odd { | ||
-fx-background: -fx-control-inner-background; | ||
} | ||
/*.table-row-cell:odd {*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reactivate this, because otherwise the rows are styled in a (ugly) strip-pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. This is a usabiltily enhancement to difference between odd and even rows. Makes it much easier to see which entry is in which row. So called "Zebra Striping"
http://uxmovement.com/content/9-design-techniques-for-user-friendly-tables/
https://alistapart.com/article/zebrastripingmoredataforthecase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also here for some general ideas about desinging data tables:
https://uxdesign.cc/design-better-data-tables-4ecc99d23356
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree as well. For the eye, it is so much help if the table has an alternating color. It goes along the same line, why we use fonts with serifs when reading a book. The eye needs guidance.
This was not an accident that I commented it out and introduced the odd/even coloring. Additionally, I made the color difference very, very small so that it is barely noticeable but still has the visual guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't want to start a rant about zebras (they are quite beautiful animals), but anyway here is the rant:
First of all, there are indeed a few studies (like this one) which show that zebra strips help users in some cases. One should note that the improvement in performance is relatively small (a few 1-3%) and only when the user is required to move vertically in a row to look for data. It is also still controversial whether the better performance comes from a superior design or just because testers where used to the striped style. On the other hand, zebra strips actually lead to worse performance when the task is to quickly identify a desired row containing given data because the eye takes these lines as fix points instead of scan quickly through the dataset.
So zebra strips may have a place in bigger datasheets where it is required to jump in a specific row and the data spans multiple columns. We have, on the other hand, a very small dataset with like 20 entries on display and 5 or so columns. Moreover, and this is probably the biggest point, a row is highlighted when selected or hovered and thus you get all the benefits of the zebra style without the clean look of the plain table.
Sorry for the rant but this is one of the things where people just follow the general advice in the form "zebra strips help to navigate in data table" without asking what are the boundary conditions for the performance increase and if they apply in the present case. This leads to a lot of documents /blog posts citing this as a always-good style to present data in a table format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets ask the other @JabRef/developers on their opinion on this. I currently find it very hard to identify an entry in the list. Especially, if you have maybe some entries in which not all required fields are visible. Compare this to the screenshot in the first post. The version from @halirutan looks much better with the stripes.
This is also the case when you have multiple files in the list view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I give my support for using zebra stripping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reacted to all but 2 things:
-
Make it possible to adjust the main-menu. I support this but there are more settings than only the colors that need to be adjustable. This should be done carefully when working on a scheme that needs these adjustments.
-
Making the entry editor background white. I'm a bit unsure if I support this visually, but I'm not generally against it. However, a white background color requires further changes because, with an entry editor that is white, we need a consistent, flat-colored border around each of the used controls. I have too less experience to implement this correctly and it would take me considerable time. Is it possible that someone else looks over this when it is really wanted to make the entry editor white?
} | ||
|
||
.disclosureNodeColumn { | ||
-fx-alignment: top-right; | ||
} | ||
|
||
.tree-table-row-cell:dragOver-bottom { | ||
-fx-border-color: #eea82f; | ||
-fx-border-color: -jr-yellow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I introduced a color for it in Main.css
and set it to our orange. This looks decent and the indicator is visible when you drag it over a marked entry.
-fx-background-color: white; | ||
} | ||
/*.tree-table-row-cell:selected > .tree-table-cell > .tree-disclosure-node > .arrow {*/ | ||
/*-fx-background-color: -jr-white;*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my tests, the commented section is the color of the small arrow, when the group is selected. This setting is not required anymore as far as I understand it because the arrow has not the same color as the icons and our markup color is bright. It looks currently like this
So for me, the arrow is always visible and looks OK even when the entry is selected.
} | ||
|
||
.table-view .column-header .glyph-icon { | ||
-fx-alignment: baseline-center; | ||
-fx-text-fill: -fx-unimportant; | ||
-fx-fill: -fx-unimportant; | ||
-fx-text-fill: -jr-icon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks pretty decent to me with the icon color and since the table header entry can be pressed to invoke the sorting action, it made sense to me to indicate this. But this might also a very personal preference.
.table-row-cell:odd { | ||
-fx-background: -fx-control-inner-background; | ||
} | ||
/*.table-row-cell:odd {*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree as well. For the eye, it is so much help if the table has an alternating color. It goes along the same line, why we use fonts with serifs when reading a book. The eye needs guidance.
This was not an accident that I commented it out and introduced the odd/even coloring. Additionally, I made the color difference very, very small so that it is barely noticeable but still has the visual guidance.
-fx-background-color: -fx-light-background; | ||
-fx-border-color: -fx-active-background; | ||
/*-fx-background-color: -fx-control-inner-background;*/ | ||
-fx-border-color: -jr-theme; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. It's only a redefinition, because the only thing I set is -fx-accent
, but I introduced a jr
variable for it.
-jr-tooltip-bg: -jr-info; | ||
-jr-tooltip-fg: -jr-black; | ||
|
||
/* Consistent size for headers of tab-pane and side-panels*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
-fx-opacity: 0.2; | ||
-fx-background-radius: 0em; | ||
} | ||
|
||
.scroll-bar:horizontal .thumb, | ||
.scroll-bar:vertical .thumb { | ||
-fx-background-color: derive(#757575, 60%); | ||
-fx-background-color: -fx-outer-border; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
container.setCenter(new Label(title)); | ||
// container.setLeft(graphic); | ||
final Label label = new Label(title); | ||
label.getStyleClass().add("sidePaneComponentHeader"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I've now merged this PR. We can fine-tune the design later and I'll provide a proposal for a dark theme in a new PR. |
…drop * upstream/maintable-beta: (48 commits) Clean unused imports Fix missing icons and wrong package for custom icons Consistent FX color scheme for JabRef (#3839) javafx replacement for file dialog (#3005) Reenable closing of entry preview by pressing Esc (#3883) Load all field editors using ViewLoader Use JabRef icons in FXML Move icon stuff to new package gui.icon Load EntryEditor using new ViewLoader Improve tooltip tests Fix import thread problem Don't use null as parameter in DialogService Make it easier to create FXML dialogs (#3880) update slf4j from 1.8.0-beta1 -> 1.8.0-beta2 New translations JabRef_en.properties (Tagalog) New translations JabRef_en.properties (Italian) New translations Menu_en.properties (Italian) New translations JabRef_en.properties (Indonesian) New translations JabRef_en.properties (Greek) New translations Menu_en.properties (Greek) ... # Conflicts: # src/main/java/org/jabref/gui/actions/CleanupAction.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java # src/main/java/org/jabref/gui/groups/GroupTreeController.java # src/main/java/org/jabref/gui/maintable/MainTable.java # src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java # src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java # src/test/java/org/jabref/logic/cleanup/MoveFilesCleanupTest.java # src/test/java/org/jabref/logic/cleanup/RenamePdfCleanupTest.java
…gsjavafx * upstream/maintable-beta: (188 commits) Fix checkstyle Exchange Ignore by Disabled (#3912) Remove all @author comments and empty method/class comments Clean unused imports Fix missing icons and wrong package for custom icons Consistent FX color scheme for JabRef (#3839) javafx replacement for file dialog (#3005) Reenable closing of entry preview by pressing Esc (#3883) Load all field editors using ViewLoader Use JabRef icons in FXML Move icon stuff to new package gui.icon Load EntryEditor using new ViewLoader Improve tooltip tests Fix import thread problem Don't use null as parameter in DialogService Make it easier to create FXML dialogs (#3880) update slf4j from 1.8.0-beta1 -> 1.8.0-beta2 New translations JabRef_en.properties (Tagalog) New translations JabRef_en.properties (Italian) New translations Menu_en.properties (Italian) ... # Conflicts: # src/main/java/org/jabref/gui/JabRefFrame.java
The PR aims to make the look and feel of JabRef more consistent by providing a well-chosen set of base colors and deriving other colors from it. At the moment, I used a flat and bright scheme with the original JabRef blue as the theme color.
In addition to the changes in the FX CSS files, I provide an additional true type font for the application icons that are not available in the Material Design Icon set. It can be loaded and used in exactly the same way as the Material Design Icons and only small changes to the code where necessary. As soon as we agreed on using this, I can make further adjustments and additions (currently, WinEd is missing) to the icons so that they fit perfectly into our toolbar. The one-colored icons should represent the original icon as much as possible and should be recognizable. The will look like this
For those who would like to help to work on this:
Our base style is
modena.css
and to make our restyling easy to use and to understand, we should try to redefine the color variables ofmodena.css
as much as possible instead of explicitly introducing and using our own variables. This means, don't use explicit color settings in the definition of styles for a control. If it is a commonly used property e.g. the border color, try to find and overwrite the appropriate modena-variable. If you indeed need a specific color, then re-use the ones defined at the top ofMain.css
.Remember that this is unfinished at the moment and I have rather commented out lines than simply deleting it.