-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add filter to reputation table #1986
Add filter to reputation table #1986
Conversation
Use it for filtering reputation score list
…omparator bindings when value is not yet set.
Add initialize and dispose methods
Adding generics for the type seems an overkill and pollutes usage.
|
||
private void addFilterMenuItem(ReputationSource reputationSource) { | ||
String title = Res.get("user.reputation.source." + reputationSource.name()); | ||
StandardTable.FilterMenuItem<ReputationListView.ListItem> filterMenuItem = new StandardTable.FilterMenuItem<>( |
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.
Using var
would be worth it here
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 tried it but then item -> item.getReputationSources()
does not recognize the type.
private final Label label = new Label(); | ||
private final ImageView defaultIcon, activeIcon; | ||
private final ContextMenu contextMenu = new ContextMenu(); | ||
private ImageView buttonIcon; | ||
@Nullable |
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.
We were trying to standarize using Optional<> instead of Nullable. Is this still the case?
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 the View classes we did not apply that because the JavaFx classes have too much nullables itself.
In that case the field cannot be null as attachListeners
is called in the constructor. I will remove the annotation therefore.
long days = getAgeInDays(date); | ||
long years = days / 365; | ||
long ageInDays = days - years * 365; | ||
String daysPostFix = ageInDays == 1 ? Res.get("temporal.day") : Res.get("temporal.days"); | ||
String dayString = ageInDays + " " + daysPostFix; | ||
if (years > 0) { | ||
String yearsPostFix = years == 1 ? Res.get("temporal.year") : Res.get("temporal.years"); | ||
String yearString = years + " " + yearsPostFix; | ||
return yearString + ", " + dayString; | ||
} else { | ||
return dayString; | ||
} |
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.
Better than manually handling plurals, can you use ChoiceFormat here?
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 for the pointer to it. I looked into ChoiceFormat but it feels like a cumbersome API to me. Would rather impl. a custom convenience method to the Res class instead... e.g.
Res.maybePlural("temporal.year", years) which maps in the method depending on years to
Res.get("temporal.year.none", years)
Res.get("temporal.year.singular", years)
Res.get("temporal.year.plural", years)
The 0 value (none) case is mostly not needed in our case.
I think such an API would be more convenient to use.
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 d801d1e
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.
ACK
Resolves #1983