Conversation
|
Thanks @mskacelik Well done! I have a few comments, which I will post tomorrow. |
| import jsinterop.annotations.JsType; | ||
|
|
||
| @JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "Object") | ||
| public class CustomFormat implements FormatOptions { |
There was a problem hiding this comment.
Maybe at some point, this class and DateTimeFormatOptions should be moved to Elemento. But for now it's ok to have them here.
| * </ul> | ||
| */ | ||
| public enum Weekday { | ||
| NARROW("narrow"), |
There was a problem hiding this comment.
I know it doesn't apply to the default Java style, but I've chosen to have enum constants in the lower camel case. IMO the code reads better when used with static imports.
| import jsinterop.annotations.JsType; | ||
|
|
||
| @JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "Object") | ||
| public class LocaleOptions implements FormatOptions { |
There was a problem hiding this comment.
Same as for CustomFormat and DateTimeFormatOptions: Maybe at some point, these classes should be moved to Elemento. But for now it's ok to have them here.
| * | ||
| * @author mskacelik | ||
| */ | ||
| public class Timestamp extends BaseComponent<HTMLElement, Timestamp> { |
There was a problem hiding this comment.
All components which have text implement org.jboss.elemento.ElementTextMethods or org.patternfly.component.ElementTextDelegate. For the timestamp component implementing ElementTextDelegate<HTMLElement, Timestamp> makes sense to me. The textDelegate() would then delegate to the time element:
@Override
public Element textDelegate() {
return timeElement.element();
}| return new Timestamp().text(text); | ||
| } | ||
|
|
||
| public static Timestamp timestamp(HTMLElement element) { |
There was a problem hiding this comment.
I think we can get rid of this factory method.
| return this; | ||
| } | ||
|
|
||
| public Timestamp is12Hour(Boolean is12Hour) { |
There was a problem hiding this comment.
Support for boolean is enough. We can remove this method.
| return utc(Boolean.valueOf(shouldDisplayUTC)); | ||
| } | ||
|
|
||
| public Timestamp utc(Boolean shouldDisplayUTC) { |
| return date; | ||
| } | ||
|
|
||
| public TimestampFormat dateFormat() { |
There was a problem hiding this comment.
I normally tend to keep the components small (if possible). I think we can get rid of all getters except the one for the date. WDYT?
There was a problem hiding this comment.
Sure, dateTime and the text (taken from ElementTextDelegate) make sense.
|
|
||
| // ------------------------------------------------------ internal | ||
|
|
||
| private void failSafeTimeElement() { |
There was a problem hiding this comment.
The time element is created in the constructor and is never removed/unset. I think we can remove this method, create and add the time element in the constructor and expect that the time element is non-null.
| * This method is called whenever any formatting property changes. | ||
| */ | ||
| private void updateDisplayAndDatetime() { | ||
| failSafeTimeElement(); |
There was a problem hiding this comment.
I think this method could be simplified in a way that it either shows the custom text or the formatted timestamp.
|
Regarding the missing navigation entry for the timestamp component in the showcase, please add/change these lines in "timestamp": {
"name": "timestamp",
"title": "Timestamp",
"route": "/components/timestamp",
"clazz": "org.patternfly.component.timestamp.Timestamp",
"illustration": "timestamp.png",
"summary": "A <b>timestamp</b> is a consistently formatted visual that displays date and time values."
},It was the missing |
|
@mskacelik Never mind the build error in |
|
@hpehl I have changed the code based on your notes and rebased the branch. Please take a look at them. With that said, I can still improve the documentation–from the javadoc side and TimestampComponent side. |
|
Thanks @mskacelik, well done! |
fixes #95
/cc @hpehl
Created a DRAFT PR for the Timestamp component. I was pretty much inspired by the API as defined by the React implementation https://www.patternfly.org/components/timestamp/ and by spec as defined by the mdn: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString.
But I still had the problem with the routing of
TimestampComponent.java. Atleast it is not rendered in the sidebar menu for both GWT and J2CL (showcase).Note: I saw that for enums (f.g, in the TitleComponent) are lower cased, should I also change them to be lowercase rather than JAVA's Upper Case approach that I used?
edit: PR's branch is up to date but the current branch is failing (due to the last commit)