Common CSS styles for spacing, padding, gaps and font-size - #16708
Conversation
github-actions
Bot
added
component: ui
component: libre-office
component: groups
component: import-load
component: preferences
component: external-files
component: maintable
component: entry-editor
component: logging
component: automatic-field-editor
component: theming
component: slr
component: ai
component: css
component: external-changes
component: welcome-tab
component: pdf-viewer
component: welcome-walkthrough
component: diff-merge
PR Summary by QodoStandardize JavaFX UI styling with reusable utility classes
AI Description
Diagram
High-Level Assessment
Files changed (80)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| Label header = new Label(Localization.lang("The following metadata changed:")); | ||
| header.getStyleClass().add("sectionHeader"); | ||
| header.getStyleClass().addAll("h4", "padding-top-12"); | ||
| container.getChildren().add(header); |
There was a problem hiding this comment.
I'm not so sure if every change is going in the right direction here. As i understand it, we abandon here semantic use of style classes in favor of pure direct styling. I' m no expert here, but shouldn't we keep the semantic style classes for the controls and map in a jabref.css the styling classes to the semantic classes?
There was a problem hiding this comment.
I would not recommend that. This new notation has multiple advantages:
- Now, we can directly see that this is a header with a bigger font size and padding on top.
Before, we need to lookupsectionHeaderin CSS. - Now everything that is styled uses the same set of styles. So it should be more consistent. What is one header has a padding of 9, another one has 11, another has 10, another has 8? While we can still have very different paddings with this change, it is now much harder to use completely different values as we provide a fixed amount (and we should decrease that - in my apps, I usually only use a padding of 2,4, rarely 8) and that works pefectly fine.
- Last one: We now use
emeverywhere and there is no morepxand new contributions should reuse that styles, so they can not make this wrong and add another new styleClass, as examplesubHeader, use a different font size and lets say 6px padding.
There was a problem hiding this comment.
Mh, my thinking was more about "reviewability". We now always have to keep in mind that these particular headers have to be h4 and padding-top-12. These headers will repeat everywhere in the app. A contributor who does not care about how an app looks will do h3 maybe, bc he thinks his header is most important, or h5 bc he is a minimalist. We don't see it in review or have to look it up if it's the right h size.
This is like in word formatting directly the whole text instead of using formatting templates. Its also not really the latex approach. Imho that should stay semantic with a mapping css. Maybe other @JabRef/developers have a different take on this?
There was a problem hiding this comment.
Maybe as a reference, in the web app world, most developers will use tailwind that work the exact same way I propose here.
The concerns you mentioned are valid, and also exist with tailwind. The solution is usually to write a already styled and ready to use component. So in our case, we could write a SectionHeader or SectionLabel class and reuse that.
There was a problem hiding this comment.
I have mixed feelings about this. But maybe I lean slightly more towards semantic reuse - is it possible to keep them "while" decreasing the number of css styles (did not go through the entire PR)?
But that may be a bit of bias because I usually don't work much with UI, so I don't think in terms of values as much as I think in terms of semantics. applyPadding(DEFAULT_HEADER_PADDING) makes code much easier for me to interpret compared to applyPadding(12), for example. If I really need to tweak it, I just look it up.
There was a problem hiding this comment.
Maybe for the few semantic cases, we could also extract a constant with the List of styleClasses and reuse that?
There was a problem hiding this comment.
I would be ok with class or constant. But to some extent, a style class is a constant too...
There was a problem hiding this comment.
dont we have a class ui constants or something?
There was a problem hiding this comment.
I would be ok with class or constant. But to some extent, a style class is a constant too...
yes, maybe just for the styles that are used often together in different code locations, maybe put into an ui constants class.
There was a problem hiding this comment.
🤖 Generated with Claude Code
Implemented the constants suggestion in f53ebaa: the recurring change-view combinations (h4 padding-2 header, font-size-090 text-subtle padding-4 legend) are now StyleClasses.CHANGE_VIEW_HEADER / CHANGE_VIEW_LEGEND in org.jabref.gui.theme.StyleClasses, used from EntryChangeDetailsView, MetadataChangeDetailsView and GitEntryChangeDetailsView. Further recurring combinations can move there as they surface.
github-actions
Bot
added
the
status: changes-required
# Conflicts: # jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChangeDetailsView.java # jabgui/src/main/java/org/jabref/gui/collab/metedatachange/MetadataChangeDetailsView.java # jabgui/src/main/resources/org/jabref/gui/theme/internal/jabref-base.css
|
🤖 Generated with Claude Code Merged
|
The .error-message rule was removed with the common-style cleanup, but ManageStudyDefinition.fxml still referenced it, leaving the blocking validation header without its danger color. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195q9VEakTp3xiXcqV5eggV
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195q9VEakTp3xiXcqV5eggV
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HKn7io1Av1DrQyqCRqEAWY
| Button jumpTo = ControlHelper.iconButton(IconTheme.JabRefIcons.LINK); | ||
| jumpTo.setTooltip(new Tooltip(Localization.lang("Jump to entry in library"))); | ||
| jumpTo.getStyleClass().add("addEntryButton"); | ||
| jumpTo.getStyleClass().addAll("addEntryButton", "h1"); |
There was a problem hiding this comment.
addEntryButton might be worth to replace as well
There was a problem hiding this comment.
🤖 Generated with Claude Code
addEntryButton cannot become a utility class — its only styling is the :selected state (transparent background because the icon color itself signals selection), which the padding/font utilities cannot express. Done in f53ebaa instead: removed the class from the plain jumpTo Button (a Button has no :selected, so it was inert there) and consolidated the two identical #importEntriesDialog/#citationRelationsTab-scoped rules into a single .addEntryButton:selected rule next to the .icon-button styles.
| if (!gridPane.getStyleClass().contains("all-fields-list")) { | ||
| gridPane.getStyleClass().add("all-fields-list"); | ||
| // The list variant sits flush in its scroll pane, unlike the padded grid of the other tabs. | ||
| gridPane.getStyleClass().remove("padding-4"); |
There was a problem hiding this comment.
should be reevaluated
There was a problem hiding this comment.
🤖 Generated with Claude Code
Reevaluated in f53ebaa: all-fields-list had no CSS rule at all (it only served as a re-entry sentinel), and padding-0 was redundant once padding-4 is removed (GridPane defaults to zero padding, nothing else pads it). Simplified to an unconditional getStyleClass().remove("padding-4"), which is idempotent on its own.
| private VBox getErrorPane() { | ||
| Label titleLabel = new Label(Localization.lang("Error")); | ||
| titleLabel.getStyleClass().add("latex-citations-error-label"); | ||
| titleLabel.getStyleClass().addAll("latex-citations-error-label", "h3", "bold"); |
There was a problem hiding this comment.
latex-citations-error-label might be worth to replace, especially all CSS styles with only a color. Maybe as follow-up
There was a problem hiding this comment.
🤖 Generated with Claude Code
Replaced in f53ebaa: the label now uses the shared text-danger utility (it is an error heading; the old -color-accent-subtle looked accidental for an error) and the #entryEditor .latex-citations-error-label rule is gone.
| xmlns="http://javafx.com/javafx/19" xmlns:fx="http://javafx.com/fxml/1" | ||
| fx:controller="org.jabref.gui.fieldeditors.journalinfo.JournalInfoView"> | ||
| <Label fx:id="title" styleClass="info-heading"> | ||
| <Label fx:id="title" styleClass="info-heading,font-size-250"> |
There was a problem hiding this comment.
info-heading, also a good candidate for dropping
There was a problem hiding this comment.
🤖 Generated with Claude Code
Dropped in f53ebaa: the FXML now uses the shared text-accent utility and the CSS rule only covers .info-label (kept — it has many usages in the same file and still maps to one shared rule).
Extract repeated change-view header/legend combinations into StyleClasses constants, drop the color-only latex-citations-error-label and info-heading classes in favor of text utilities, consolidate the duplicated addEntryButton:selected rules, and simplify the AllFieldsTab padding tweak. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195q9VEakTp3xiXcqV5eggV
| HBox info = new HBox(8); | ||
| HBox.setHgrow(info, Priority.ALWAYS); | ||
| info.getStyleClass().add("linked-files-info"); // To align with buttons below which also have 0.5em padding | ||
| info.getStyleClass().add("padding-6-0"); // To align with buttons below which also have 0.5em padding |
There was a problem hiding this comment.
Discussion:
semantic class ("linked-files-info")
versus
layout-classes ("padding-6-0")
I think, https://getbootstrap.com/docs/5.3/getting-started/introduction/ is more on the latter.
There was a problem hiding this comment.
My opinion (rephrased from #16708 (comment)) - web development frameworks like bootstrap or tailwind will always lean towards that convention as the flow for building website is generally different from building a desktop app. They come more from the world of "hot reload"/instant render where you keep adjusting the raw values as per visible results till satisfied. It is not from a reusability/review-ability or maintainability first perspective.
Any maintainability that people start thinking about when building websites starts from the javascript layer.
There was a problem hiding this comment.
bootstrap also has layout classes, yes. See e.g. for container: https://getbootstrap.com/docs/5.3/layout/containers/
Desktop and Web development are different, but JavaFX is very close. And adding or removing a styleClass like h4 in code will make it also easier to hot-reload for us as well. Because we usually never need to adjust the CSS, rather the code or fxml. Both very easy to hot-reload.
Another minor advantage is also, that people coming from Web will immediately understand it, since it is inspired by the CSS Utility frameworks.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195q9VEakTp3xiXcqV5eggV
github-actions
Bot
added
the
component: external-application-integration
|
Maybe we can find a middle way: semantic, but as general as possible. Instead of "linked-files-header" or "preferences-tab-whatever" use "dialog-header" and so on. |
Yeah that makes sense, especially for more than one style. |
…tyle # Conflicts: # jabgui/src/main/java/org/jabref/gui/git/GitEntryChangeDetailsView.java # jabgui/src/main/java/org/jabref/gui/newentry/NewEntryView.java # jabgui/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTab.java # jabgui/src/main/java/org/jabref/gui/preferences/keybindings/KeyBindingsTab.java
jabref-machine
added
status: ready-for-review
github-actions
Bot
added
the
status: to-be-merged
* upstream/main: Common CSS styles for spacing, padding, gaps and font-size (JabRef#16708) Group file operations in the entry context menu (JabRef#16829) add subset search (JabRef#16871) Speed up building (JabRef#16873) Enhance developer documentation (JabRef#16760) Fix BST entry preview formatting (JabRef#16853) Fix invalid regex handling in search (JabRef#16855) New Crowdin updates (JabRef#16867) # Conflicts: # jabgui/src/main/java/org/jabref/gui/collab/metedatachange/MetadataChangeDetailsView.java
* upstream/main: fix(bibtex): recover after unmatched braces (#16869) Common CSS styles for spacing, padding, gaps and font-size (#16708) Group file operations in the entry context menu (#16829) add subset search (#16871) Speed up building (#16873) Enhance developer documentation (#16760) Fix BST entry preview formatting (#16853)
* main: (66 commits) Fix undo C - One undomanager / journal per library and some cleanups (#16857) Fix status label update for reviews on fork PRs (#16883) Rework shared SQL database synchronization (PostgreSQL, live updates) (#11879) Gracefully handle JGit errors (#16882) Require SHA-pinned GitHub Actions in AGENTS.md (#16875) fix(ai): keep group chat window after library edits (#16879) Add auto-commit, push & pull features for Git (#16651) Pin issue only when the PR author is the assignee (#16872) Select newly added entry (#16845) Add infer style button (#16870) Show group changes in Git diff (#16868) Label CI/CD and build-system pull requests automatically (#16877) AsyncEmbeddingModel.java: Switch NotNull annotation to NonNull from jspecify. (#16880) fix(bibtex): recover after unmatched braces (#16869) Common CSS styles for spacing, padding, gaps and font-size (#16708) Group file operations in the entry context menu (#16829) add subset search (#16871) Speed up building (#16873) Enhance developer documentation (#16760) Fix BST entry preview formatting (#16853) ... # Conflicts: # docs/requirements/shared-database.md # jabgui/src/main/java/org/jabref/gui/autosaveandbackup/AutosaveManager.java # jabgui/src/main/java/org/jabref/gui/shared/SharedDatabaseLoginDialogView.java # jabgui/src/main/java/org/jabref/gui/shared/SharedDatabaseLoginDialogViewModel.java # jabgui/src/main/java/org/jabref/gui/shared/SharedDatabaseUIManager.java # jabgui/src/main/resources/org/jabref/gui/shared/SharedDatabaseLoginDialog.fxml # jablib/src/main/java/org/jabref/logic/shared/DBMSConnectionProperties.java # jablib/src/main/java/org/jabref/logic/shared/DBMSConnectionPropertiesBuilder.java # jablib/src/main/java/org/jabref/logic/shared/prefs/SharedDatabasePreferences.java
…list origin/common-style (JabRef#16708), origin/fulltext-context-menu (JabRef#16829) and origin/fulltext-download-background-task (JabRef#16831) no longer exist upstream; last night's run warned about all three. Upstream has landed each of them itself: the StyleClasses migration, the GroupChangeDetailsView extraction and the background full text search (req~fetchers.fulltext-background-search~1) all arrived via origin/main, so experimental keeps the work without the branches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEfEAwaU63aKf2XzAKa7p8
6 participants
Summary
Decreases the amount of CSS styles.
That is around ~600 lines less CSS code, because all of them can now be expressed with some common style classes.
Beside less CSS code, the other advantage is that we have some rules and common spacings. So the UI will look more unified.
From my experience, many contributors added more and more styleClasses just for some padding. They now should use the common styleClasses (or we should tell them in the PR review otherwise).
Steps to test
The whole JabRef application is affected
Related issues and pull requests
Closes #16042
AI usage
Reviewed by AI.
Checklist
CHANGELOG.mddescribing the change from the user's point of view (if the change is visible to the user)