Updated time filter to use gemstone version #97
Conversation
Tenacs
force-pushed
the
Updated-TimeFilter-to-use-gemstone-version
branch
from
e3adc0e to
258ae81
Compare
collins-self
force-pushed
the
Updated-TimeFilter-to-use-gemstone-version
branch
from
8f1966b to
2b90980
Compare
There was a problem hiding this comment.
I think you are being a little over-zealous on what formating constants you replace. I don't think you can use one global constant everywhere since different components expect different formats and different formats make sense in different places. I left some comments on some places where I saw this done but I know for sure I didn't comment on everything. Can you go through and only replace where it makes sense for the setting? What is the setting meant to unifiy? The server format we get back from the controllers? That we send to the controllers? Clarification on this would assist me in reviewing this PR.
| setEventMarkers(data.map(datum => { | ||
| const meterID = props.Plot.Channels.find(channel => channel.MeterKey === datum["Meter Key"]).MeterID; | ||
| return { value: moment.utc(datum.Time, eventFormat).valueOf(), meterID: meterID, eventID: datum["EventID"]} | ||
| return { value: moment.utc(datum.Time, dateTimeFormat).valueOf(), meterID: meterID, eventID: datum["EventID"] } |
There was a problem hiding this comment.
Event format is different than date time
| const startMoment: moment.Moment = moment.utc(props.TimeFilter.start, dateTimeFormat); | ||
| const endMoment: moment.Moment = moment.utc(props.TimeFilter.end, dateTimeFormat); | ||
|
|
||
| const startTime: number = startMoment.valueOf(); | ||
| const endTime: number = endMoment.valueOf(); |
There was a problem hiding this comment.
| const startMoment: moment.Moment = moment.utc(props.TimeFilter.start, dateTimeFormat); | |
| const endMoment: moment.Moment = moment.utc(props.TimeFilter.end, dateTimeFormat); | |
| const startTime: number = startMoment.valueOf(); | |
| const endTime: number = endMoment.valueOf(); | |
| const startTime: number = moment.utc(props.TimeFilter.start, dateTimeFormat).valueOf(); | |
| const endTime: number = moment.utc(props.TimeFilter.end, dateTimeFormat).valueOf(); |
No need to have this assignment, it makes it a little more hard to read in my opinion.
| const [trendMarkers, setTrendMarkers] = React.useState<TrendSearch.IMarker[]>([]); | ||
| const [sortField, setSortField] = React.useState<string>('MeterName'); | ||
| const [ascending, setAscending] = React.useState<boolean>(true); | ||
| const momentFormat = "DD HH:mm:ss.SSS"; |
There was a problem hiding this comment.
This replacement doesn't work, moment format isn't the same as datetimeformat from settings.
| /> | ||
| <ReportTimeFilter filter={props.Plot.TimeFilter} showQuickSelect={false} | ||
| setFilter={newFilter => props.SetPlot({ ...props.Plot, TimeFilter: newFilter })} /> | ||
| <TimeFilter filter={props.Plot.TimeFilter/* convertTimeFilter(props.Plot.TimeFilter) */} showQuickSelect={false} |
There was a problem hiding this comment.
What's with this commented out code?
|
|
||
| React.useEffect(() => { | ||
| let range = ""; | ||
| const startMoment = moment(timeFilter.start); // These default to the date+time format |
There was a problem hiding this comment.
Please don't use default moment formats, it can lead to unexpected bugs. Be explicit
| { | ||
| Value: 'center', | ||
| Label: 'Center Date/Time and Window', | ||
| }, |
There was a problem hiding this comment.
Why are we dropping support for centered time window format?
There was a problem hiding this comment.
I believe that was the main update I was doing to this branch. Christoph said to do away with it, and just handle its conversion to the back end. (If it's still using that format in the back end)
collins-self
force-pushed
the
Updated-TimeFilter-to-use-gemstone-version
branch
from
96f0333 to
ab73e5d
Compare
4 participants
Removes 'center' time setting and standardizes date time format in SettingSlice. Cleaned up TimeFilter and date and time usage throughout.
Depends on GridProtectionAlliance/gpa-gemstone#321