-
Notifications
You must be signed in to change notification settings - Fork 438
Extended chart link to support from latest readings #1530
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
base: development
Are you sure you want to change the base?
Changes from all commits
2ef5c6e
7ff4d7c
a6c1069
79f02c5
a8b2782
de1a067
7e3d0e6
09005f6
6ac0799
89b9884
01fb5e1
d985ffe
94a1e64
f68f589
e4864eb
135b53f
8ffc441
84bb65b
4c2c367
d4e5b61
e79ee5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| { | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file only has new blank lines so please revert changes so none in file. |
||
| { | ||
|
|
||
| "name": "Attach to Node in Docker", | ||
| "type": "node", | ||
| "request": "attach", | ||
|
|
@@ -44,6 +46,7 @@ | |
| "Attach to Node in Docker", | ||
| "Launch Firefox against localhost" | ||
| ] | ||
|
|
||
| } | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [31morigin/HEAD[m -> origin/development | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure what this file is for. Should it be removed? |
||
| [31morigin/ann_branch[m | ||
| [31morigin/ann_branch_2[m | ||
| [31morigin/ann_will_branch[m | ||
| [31morigin/codebase-testing[m | ||
| [31morigin/development[m | ||
| [31morigin/w/dev[m | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [31morigin/HEAD[m -> origin/development | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure what this file is for. Should it be removed? |
||
| [31morigin/ann_branch[m | ||
| [31morigin/ann_branch_2[m | ||
| [31morigin/ann_will_branch[m | ||
| [31morigin/codebase-testing[m | ||
| [31morigin/development[m | ||
| [31morigin/w/dev[m | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,15 +5,28 @@ | |
| import * as React from 'react'; | ||
| import { toast } from 'react-toastify'; | ||
| import ReactTooltip from 'react-tooltip'; | ||
| import { Button, ButtonGroup, Input } from 'reactstrap'; | ||
| import { Button, ButtonGroup,Input} from 'reactstrap'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. space after comma. |
||
| import { useAppDispatch, useAppSelector } from '../redux/reduxHooks'; | ||
| import { selectChartLink } from '../redux/selectors/uiSelectors'; | ||
| import { selectChartLinkHideOptions, setChartLinkOptionsVisibility } from '../redux/slices/appStateSlice'; | ||
| import { selectSelectedGroups, selectSelectedMeters } from '../redux/slices/graphSlice'; | ||
| import { showErrorNotification, showInfoNotification } from '../utils/notifications'; | ||
| import { | ||
| selectChartLinkHideOptions, selectIsKeepCurrent, setChartLinkOptionsVisibility, | ||
| setIsKeepCurrent | ||
| } from '../redux/slices/appStateSlice'; | ||
| import { | ||
| selectSelectedGroups, | ||
| selectSelectedMeters, | ||
| selectQueryTimeInterval | ||
| } from '../redux/slices/graphSlice'; | ||
| import { | ||
| showErrorNotification, | ||
| showInfoNotification | ||
| } from '../utils/notifications'; | ||
| import { useTranslate } from '../redux/componentHooks'; | ||
| import TooltipMarkerComponent from './TooltipMarkerComponent'; | ||
| import { wellStyle, rowFlexStart } from '../styles/modalStyle'; | ||
| import { wellStyle, rowFlexStart, labelStyle } from '../styles/modalStyle'; | ||
| import { checkboxStyle } from '../styles/modalStyle'; | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove new blank lines so only one. |
||
|
|
||
|
|
||
| /** | ||
| * @returns chartLinkComponent | ||
|
|
@@ -26,59 +39,134 @@ export default function ChartLinkComponent() { | |
| const linkHideOptions = useAppSelector(selectChartLinkHideOptions); | ||
| const selectedMeters = useAppSelector(selectSelectedMeters); | ||
| const selectedGroups = useAppSelector(selectSelectedGroups); | ||
| const queryTimeInterval = useAppSelector(selectQueryTimeInterval); | ||
| const isKeepCurrent = useAppSelector(selectIsKeepCurrent); | ||
| const ref = React.useRef<HTMLDivElement>(null); | ||
|
|
||
| // THIS react.UseMemo ONLY RETURNS TRUE WHEN THE CONDITIONS ARE MET FOR USING KEEP CURRENT (left is is bounded and right is unbounded) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the uppercase needed? I think you are trying to emphasize the point but a regular comment seems fine as there are lots of comments on the code throughout OED. Some people find uppercase to be shouting so I try to avoid. |
||
| const shouldShowKeepCurrentCheckbox = React.useMemo(() => { | ||
| if (!queryTimeInterval) return false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if the logic could be made simpler and clearer since you only want to return true in one case. What if you set a variable to false. Then check if the left is bounded and right unbounded (protecting against undefined which is also false) and set variable to true. (A trinary as a single statement to do this is also fine.) Then return the variable value. I note this might avoid Multiple returns that, I believe, research has shown can lead to errors. It may not matter after the changes but OED wants braces around all if/else statements, even if a single one for clarity. |
||
| if (queryTimeInterval.getIsBounded()) return false; | ||
| if ( | ||
| queryTimeInterval.getStartTimestamp() == null && | ||
| !queryTimeInterval.getIsBounded() | ||
| ) | ||
| return false; | ||
| return true; | ||
| }, [queryTimeInterval]); | ||
|
|
||
| const handleButtonClick = () => { | ||
| // First attempt to write directly to user's clipboard. | ||
| navigator.clipboard.writeText(linkText) | ||
| navigator.clipboard | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I preferred the old formatting without the extra lines to shorten the code. |
||
| .writeText(linkText) | ||
| .then(() => { | ||
| showInfoNotification(translate('clipboard.copied'), toast.POSITION.TOP_RIGHT, 1000); | ||
| showInfoNotification( | ||
| translate('clipboard.copied'), | ||
| toast.POSITION.TOP_RIGHT, | ||
| 1000 | ||
| ); | ||
| }) | ||
| .catch(() => { | ||
| // if operation fails, open copyable text for manual copy. | ||
| showErrorNotification(translate('clipboard.not.copied'), toast.POSITION.TOP_RIGHT, 1000); | ||
| showErrorNotification( | ||
| translate('clipboard.not.copied'), | ||
| toast.POSITION.TOP_RIGHT, | ||
| 1000 | ||
| ); | ||
| setLinkTextVisible(true); | ||
| }); | ||
| }; | ||
| if (selectedMeters.length > 0 || selectedGroups.length > 0) { | ||
| return ( | ||
| <div> | ||
| {/* inputting new "keep chart current" feature */} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment should just say what the code does or be removed if you think it is not necessary. |
||
| <div style={labelStyle}>{translate('chart.link.options.title')}</div> | ||
| {/* hide options checkbox */} | ||
| <div className="checkbox"> | ||
| <Input | ||
| type="checkbox" | ||
| style={checkboxStyle} | ||
| defaultChecked={linkHideOptions} | ||
| onClickCapture={e => { | ||
| e.stopPropagation(); | ||
| dispatch(setChartLinkOptionsVisibility(!linkHideOptions)); | ||
| }} | ||
| onMouseOver={() => { | ||
| ref.current && ReactTooltip.show(ref.current); | ||
| }} | ||
| onMouseLeave={() => { | ||
| ref.current && ReactTooltip.hide(ref.current); | ||
| }} | ||
| /> | ||
| <label>{translate('hide.options.when.using.this.label')}</label> | ||
| <TooltipMarkerComponent | ||
| page="home" | ||
| helpTextId="help.home.toggle.chart.link" | ||
| /> | ||
| </div> | ||
| {/* keep current checkbox ----> */} | ||
| {/* USE shouldShowKeepCurrentCheckbox AS THE CONDITIONAL BOOLEAN */} | ||
| <div className="checkbox"> | ||
| <Input | ||
| type="checkbox" | ||
| style={checkboxStyle} | ||
| onMouseOver={() => { | ||
| ref.current && ReactTooltip.show(ref.current); | ||
| }} | ||
| onMouseLeave={() => { | ||
| ref.current && ReactTooltip.hide(ref.current); | ||
| }} | ||
| checked={isKeepCurrent} | ||
| onChange={e => dispatch(setIsKeepCurrent(e.target.checked))} | ||
| disabled={!shouldShowKeepCurrentCheckbox} | ||
| // shouldShow is the value we use to tell if it should be disabled or not | ||
| //when disabled = true, you CANNOT click the checkbox | ||
| /> | ||
|
|
||
| <label | ||
| // uses the default value when shouldShowKeepCurrentCheckbox is true, and the greyed out color when it's false | ||
| style={{ | ||
| color: shouldShowKeepCurrentCheckbox | ||
| ? undefined | ||
| : 'hsl(0, 0%, 70%)' | ||
| }} | ||
| > | ||
| {translate('keep.chart.current.label')} | ||
| </label> | ||
| {/* we need to create a tool tip for "keep chart current" checkbox */} | ||
| <TooltipMarkerComponent page="home" helpTextId="help.home.toggle.chart.link.keep.current" /> | ||
| </div> | ||
|
|
||
| <div style={rowFlexStart}> | ||
| <ButtonGroup > | ||
| <Button outline onClick={handleButtonClick} > | ||
| <div style={{ display: 'flex', flexDirection: 'row', justifyContent: 'space-evenly', gap: '1em', alignItems: 'center' }}> | ||
|
|
||
| <ButtonGroup> | ||
| <Button outline onClick={handleButtonClick}> | ||
| <div | ||
| style={{ | ||
| display: 'flex', | ||
| flexDirection: 'row', | ||
| justifyContent: 'space-evenly', | ||
| gap: '1em', | ||
| alignItems: 'center' | ||
| }} | ||
| > | ||
| {translate('chart.link')} | ||
| <div ref={ref} data-for={'home'} data-tip={'help.home.toggle.chart.link'} > | ||
| <Input type='checkbox' defaultChecked={linkHideOptions} | ||
| onClickCapture={e => { | ||
| e.stopPropagation(); | ||
| dispatch(setChartLinkOptionsVisibility(!linkHideOptions)); | ||
| }} | ||
| onMouseOver={() => { | ||
| ref.current && ReactTooltip.show(ref.current); | ||
| }} | ||
| onMouseLeave={() => { | ||
| ref.current && ReactTooltip.hide(ref.current); | ||
| }} | ||
| /> | ||
| </div> | ||
|
|
||
| </div> | ||
| </Button> | ||
| <Button outline onClick={() => setLinkTextVisible(visible => !visible)}> | ||
| <Button | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the original formatting please be put back since shorter and will remove these lines as having changes. |
||
| outline | ||
| onClick={() => setLinkTextVisible(visible => !visible)} | ||
| > | ||
| {linkTextVisible ? 'x' : 'v'} | ||
| </Button> | ||
|
|
||
| </ButtonGroup> | ||
| <TooltipMarkerComponent page='home' helpTextId='help.home.toggle.chart.link' /> | ||
| </div> | ||
| { | ||
| linkTextVisible && | ||
| <div style={wellStyle}> | ||
| {linkText} | ||
| </div> | ||
| } | ||
| </div > | ||
| {linkTextVisible && <div style={wellStyle}>{linkText}</div>} | ||
| </div> | ||
| ); | ||
| } | ||
| else { | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import { | |
| changeSliderRange, selectChartToRender, selectHistoryIsDirty, | ||
| selectSelectedGroups, selectSelectedMeters, | ||
| selectSliderRangeInterval, selectInitialXAxisRange, | ||
| selectQueryTimeInterval, updateTimeIntervalAndSliderRange | ||
| selectQueryTimeInterval, updateTimeIntervalAndSliderRange,updateTimeCreated | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. space after comma. |
||
| } from '../redux/slices/graphSlice'; | ||
| import HistoryComponent from './HistoryComponent'; | ||
| import { ChartTypes } from '../types/redux/graph'; | ||
|
|
@@ -110,6 +110,7 @@ export const RefreshGraphComponent = () => { | |
| const maxX = initialXAxisRange?.getEndTimestamp?.(); | ||
| const nextInterval = getNextQueryTimeInterval(queryTimeInterval, sliderInterval, minX, maxX); | ||
| dispatch(updateTimeIntervalAndSliderRange(nextInterval)); | ||
| dispatch(updateTimeCreated()); | ||
| } | ||
| }} | ||
| /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import { LanguageTypes } from 'types/redux/i18n'; | |
| import { selectGroupDataById } from '../../redux/api/groupsApi'; | ||
| import { selectMeterDataById } from '../../redux/api/metersApi'; | ||
| import { selectUnitDataById } from '../../redux/api/unitsApi'; | ||
| import { selectChartLinkHideOptions, selectSelectedLanguage } from '../../redux/slices/appStateSlice'; | ||
| import { selectChartLinkHideOptions, selectIsKeepCurrent, selectSelectedLanguage } from '../../redux/slices/appStateSlice'; | ||
| import { DataType } from '../../types/Datasources'; | ||
| import { GroupedOption, SelectOption } from '../../types/items'; | ||
| import { ChartTypes, ShiftAmount } from '../../types/redux/graph'; | ||
|
|
@@ -453,9 +453,10 @@ export const selectChartLink = createAppSelector( | |
| selectGraphState, | ||
| selectChartLinkHideOptions, | ||
| selectSliderRangeInterval, | ||
| selectIsKeepCurrent, | ||
| state => state.maps.selectedMap | ||
| ], | ||
| (current, chartLinkHideOptions, rangeSliderInterval, selectedMap) => { | ||
| (current, chartLinkHideOptions, rangeSliderInterval, isKeepCurrent,selectedMap) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spacing off - see other comment. |
||
| // Determine the beginning of the URL to add arguments to. | ||
| // This is the current URL. | ||
| const winLocHref = window.location.href; | ||
|
|
@@ -514,6 +515,12 @@ export const selectChartLink = createAppSelector( | |
| if (chartLinkHideOptions) { | ||
| linkText += '&optionsVisibility=false'; | ||
| } | ||
| if (isKeepCurrent) { | ||
| const timeCreatedEnd = current.timeCreated.getEndTimestamp(); | ||
| const sliderStart = current.rangeSliderInterval.getStartTimestamp(); | ||
| const diffDays = timeCreatedEnd.diff(sliderStart, 'days', true); | ||
| linkText += `&timeSpan=${diffDays.toFixed(2)}`; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious about the |
||
| } | ||
| return linkText; | ||
| } | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ export interface AppState { | |
| selectedLanguage: LanguageTypes; | ||
| languageManuallySet: boolean; | ||
| refreshingReadings: boolean; | ||
| isKeepCurrent: boolean; | ||
| } | ||
|
|
||
| const defaultState: AppState = { | ||
|
|
@@ -33,7 +34,8 @@ const defaultState: AppState = { | |
| selectedLanguage: LanguageTypes.en, | ||
| chartLinkHideOptions: false, | ||
| languageManuallySet: false, | ||
| refreshingReadings: false | ||
| refreshingReadings: false, | ||
| isKeepCurrent: false | ||
| }; | ||
|
|
||
| export const appStateSlice = createThunkSlice({ | ||
|
|
@@ -63,6 +65,9 @@ export const appStateSlice = createThunkSlice({ | |
| setRefresingReadings: create.reducer<boolean>((state, action) => { | ||
| state.refreshingReadings = action.payload; | ||
| }), | ||
| setIsKeepCurrent: create.reducer<boolean>((state, action) => { | ||
| state.isKeepCurrent = action.payload; | ||
| }), | ||
| initApp: create.asyncThunk( | ||
| // Thunk initiates many data fetching calls on startup before react begins to render | ||
| async (_: void, { dispatch }) => { | ||
|
|
@@ -132,7 +137,9 @@ export const appStateSlice = createThunkSlice({ | |
| selectOptionsVisibility: state => state.optionsVisibility, | ||
| selectSelectedLanguage: state => state.selectedLanguage, | ||
| selectChartLinkHideOptions: state => state.chartLinkHideOptions, | ||
| selectRefreshingReadings: state => state.refreshingReadings | ||
| selectRefreshingReadings: state => state.refreshingReadings, | ||
| selectIsKeepCurrent: state => state.isKeepCurrent | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove extra blank line. |
||
| } | ||
| }); | ||
|
|
||
|
|
@@ -143,13 +150,15 @@ export const { | |
| setOptionsVisibility, | ||
| updateSelectedLanguage, | ||
| setChartLinkOptionsVisibility, | ||
| setRefresingReadings | ||
| setRefresingReadings, | ||
| setIsKeepCurrent | ||
| } = appStateSlice.actions; | ||
|
|
||
| export const { | ||
| selectInitComplete, | ||
| selectOptionsVisibility, | ||
| selectSelectedLanguage, | ||
| selectChartLinkHideOptions, | ||
| selectRefreshingReadings | ||
| selectRefreshingReadings, | ||
| selectIsKeepCurrent | ||
| } = appStateSlice.selectors; | ||
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 file is used by OED to define the debugging setup, etc. I'm not sure it should be in .gitignore.