Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove schedule events duplicate requests and ignore window.resize #4307

Merged
merged 7 commits into from
May 20, 2024
Merged

Remove schedule events duplicate requests and ignore window.resize #4307

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b73ead2
remove calling refreshEvents in ScheduleFinal on every selectedTimezo…
maskin25 May 3, 2024
05fa76e
remove rotation form hide on window resize
maskin25 May 3, 2024
9a3024a
refresh schedules events on UserTimezoneSelct onChange
maskin25 May 10, 2024
3ded316
Merge branch 'dev' into maxim/remove-duplicate-schedule-events-requests
maskin25 May 10, 2024
030d9fc
Merge branch 'dev' into maxim/remove-duplicate-schedule-events-requests
teodosii May 16, 2024
3511a09
onResize calculation
teodosii May 16, 2024
69ad632
lint
teodosii May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 0 additions & 11 deletions grafana-plugin/src/containers/RotationForm/RotationForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,13 +480,6 @@ export const RotationForm = observer((props: RotationFormProps) => {
}
}, [store.timezoneStore.selectedTimezoneOffset]);

useEffect(() => {
window.addEventListener('resize', onResize);
return () => {
window.removeEventListener('resize', onResize);
};
}, []);

Comment on lines -483 to -489
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was there maybe a valid reason why we called onHide on window resize? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen.Recording.2024-05-10.at.11.09.30.mov

I found the only case when window resizng can be an issue (see video attached), in that case user can just resize window back, it's better than just close the modal on any resize

Copy link
Member

@teodosii teodosii May 13, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should add some calculations if we remove the resize handler. When that happens it's not very clear for the user that the resize triggered that behavior, and we will run into escalations again. Either we

  • close the modal on resizing (no one on prod environment is expected really to open the devtools)
  • OR figure out boundaries for the modal on each resize event, and eventually, if it doesn't fit, you'll still need to close it, so it's basically the first option with 1 extra step

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way I think some implementation needs to be added when the browser is being narrowed, otherwise users will run into scenario where they cannot close/submit the modal anymore due to vertical/horizontal restrains.

const isFormValid = useMemo(() => !Object.keys(errors).length, [errors]);

const hasUpdatedShift = shift && shift.updated_shift;
Expand Down Expand Up @@ -765,10 +758,6 @@ export const RotationForm = observer((props: RotationFormProps) => {
</>
);

function onResize() {
onHide();
}

function onDraggableInit(_e: DraggableEvent, data: DraggableData) {
if (!data) {
return;
Expand Down
9 changes: 2 additions & 7 deletions grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { FC, useEffect } from 'react';
import React, { FC } from 'react';

import { HorizontalGroup } from '@grafana/ui';
import cn from 'classnames/bind';
Expand Down Expand Up @@ -40,8 +40,7 @@ interface ScheduleFinalProps extends WithStoreProps {
const _ScheduleFinal: FC<ScheduleFinalProps> = observer(
({ store, simplified, scheduleId, filters, onShowShiftSwapForm, onShowOverrideForm, onSlotClick }) => {
const {
timezoneStore: { currentDateInSelectedTimezone, calendarStartDate, selectedTimezoneOffset },
scheduleStore: { refreshEvents },
timezoneStore: { currentDateInSelectedTimezone, calendarStartDate },
} = store;
const base = 7 * 24 * 60; // in minutes
const diff = currentDateInSelectedTimezone.diff(calendarStartDate, 'minutes');
Expand All @@ -62,10 +61,6 @@ const _ScheduleFinal: FC<ScheduleFinalProps> = observer(
onShowOverrideForm('new', shiftStart, shiftEnd);
};

useEffect(() => {
refreshEvents(scheduleId);
}, [selectedTimezoneOffset]);

Comment on lines -65 to -67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this was refetching events if you changed the timezone dropdown and this will no longer be the case? Is there any negative consequences of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since your comment I've added refreshing schedules events on TimezoneSelect onChange callback, it settles things

return (
<div className={cx('root')}>
{!simplified && (
Expand Down
12 changes: 7 additions & 5 deletions grafana-plugin/src/containers/UserTimezoneSelect/UserTimezoneSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ interface TimezoneOption {

interface UserTimezoneSelectProps {
scheduleId?: string;
onChange: (value: number) => void;
}

export const UserTimezoneSelect: FC<UserTimezoneSelectProps> = observer(({ scheduleId }) => {
export const UserTimezoneSelect: FC<UserTimezoneSelectProps> = observer(({ scheduleId, onChange }) => {
const store = useStore();
const users = UserHelper.getSearchResult(store.userStore).results || [];

Expand Down Expand Up @@ -107,9 +108,6 @@ export const UserTimezoneSelect: FC<UserTimezoneSelectProps> = observer(({ sched
description: '',
},
]);

store.timezoneStore.setSelectedTimezoneOffset(utcOffset);
store.scheduleStore.refreshEvents(scheduleId);
}
},
[options]
Expand All @@ -119,7 +117,11 @@ export const UserTimezoneSelect: FC<UserTimezoneSelectProps> = observer(({ sched
<div className={cx('root')} data-testid="timezone-select">
<Select
value={selectedOption}
onChange={(option) => store.timezoneStore.setSelectedTimezoneOffset(option.value)}
onChange={(option) => {
store.timezoneStore.setSelectedTimezoneOffset(option.value);

onChange(option.value);
}}
width={30}
options={options}
filterOption={filterOption}
Expand Down
2 changes: 1 addition & 1 deletion grafana-plugin/src/pages/schedule/Schedule.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ class _SchedulePage extends React.Component<SchedulePageProps, SchedulePageState
{users && (
<HorizontalGroup>
<Text type="secondary">Current timezone:</Text>
<UserTimezoneSelect scheduleId={scheduleId} />
<UserTimezoneSelect scheduleId={scheduleId} onChange={this.handleDateRangeUpdate} />
</HorizontalGroup>
)}
<HorizontalGroup>
Expand Down
12 changes: 8 additions & 4 deletions grafana-plugin/src/pages/schedules/Schedules.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class _SchedulesPage extends React.Component<SchedulesPageProps, SchedulesPageSt
<HorizontalGroup justify="space-between">
<Text.Title level={3}>Schedules</Text.Title>
<div className={cx('schedules__actions')}>
<UserTimezoneSelect />
<UserTimezoneSelect onChange={this.refreshExpandedSchedules} />
<WithPermissionControlTooltip userAction={UserActions.SchedulesWrite}>
<Button variant="primary" onClick={this.handleCreateScheduleClick}>
+ New schedule
Expand Down Expand Up @@ -171,12 +171,16 @@ class _SchedulesPage extends React.Component<SchedulesPageProps, SchedulesPageSt
const index = expandedRowKeys.indexOf(data.id);
const newExpandedRowKeys = [...expandedRowKeys];
newExpandedRowKeys.splice(index, 1);
this.setState({ expandedRowKeys: newExpandedRowKeys }, () => {
this.props.store.scheduleStore.refreshEvents(data.id);
});
this.setState({ expandedRowKeys: newExpandedRowKeys });
}
};

refreshExpandedSchedules = () => {
const { expandedRowKeys } = this.state;

expandedRowKeys.forEach(this.props.store.scheduleStore.refreshEvents);
};

renderSchedule = (data: Schedule) => (
<div className={cx('schedule')}>
<TimelineMarks />
Expand Down