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 disable editor #343

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Remove disable editor #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 3 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ All props are _optional_
| viewerExtraComponent | Function(fields, event) OR Component. Additional component in event viewer popper |
| viewerTitleComponent | Function(event). Helper function to render custom title in event popper |
| disableViewer | boolean. If true, the viewer popover will be disabled globally |
| disableEditor | boolean. If true, the editor modal will be disabled globally |
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the use of editable could prevent the opening of the editor, at the same time my intention was to provide a way to click a grid cell, intercept the event and attach a custom logic.

Unfortunately with editable=false the recently introduced onCellClick is not called any more.

At the moment the only way I have to intercept the click is to implement a custom editor and close it programmatically (something similar of this hack on viewer side: #329 (comment)) and having the screen flashing for the backdrop appearing and disappearing immediatly.

Copy link
Owner Author

@aldabil21 aldabil21 Apr 9, 2024 •
edited

Choose a reason for hiding this comment

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

it seems to me that making it focusable without any click result (the popover) is unusual, it should not be the case.

I think you can resolve this using tabIndex of -1 if its disabled

can you open a PR for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Before doing any new PR I'd like to explain my needs so we can find the more appropriate way to implement ;)

In some scenarios I need to use the calendar disabling completely the default modal / popover. If a user click an event I only need a callback with event details. If a user click a cell on the grid I only need the cell details (start, end, resource...). I need also that in this case the grid works as usual (click, focus, and so on).

disableViewer was a good way to do it but no events were raised, because of the button disabled so in PR #338 I addressed a fix but now in this PR the fix was reverted.

I think buttons should not be disabled when disableViewer=true because we are asking the component to disable the popover, not events.

The same IMHO should be done for the editor: I need a way to disable completely the popover and give the possibility to the developer to implement its own logic.

I'll give you an example of why I am asking this: I need to fill some dates in a component of my application and such dates come from the interaction with events or the grid.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm trying to make things generic and not for specific needs

IMO disabled button should not be focusable

To go around this and fulfill your needs, we can apply tabIndex={-1} without disable. In this case, the button will not be disabled, and also not focusable.

Does this sounds good for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try and do some experiments before merging so we can discuss on it. Can you open the PR (I'm not sure it's 100% clear for me)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Check #344

Copy link
Contributor

@slash-84 slash-84 Apr 10, 2024 •
edited

Choose a reason for hiding this comment

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

Ok, regression on clicks has disappeared.

I have another issue: with editable={false} I also lose the ability to drag & drop, is there a way to restore it?

BTW for the moment we can merge

Copy link
Contributor

@slash-84 slash-84 Apr 10, 2024 •
edited

Choose a reason for hiding this comment

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

I have another issue: with editable={false} I also lose the ability to drag & drop, is there a way to restore it?

have a look #346

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok, regression on clicks has disappeared.

I have another issue: with editable={false} I also lose the ability to drag & drop, is there a way to restore it?

BTW for the moment we can merge

This is intended. drag & drop is an edit action. if editable is false, then it should not be dragged

| resources | Array. Resources array to split event views with resources <br>_Example_ <pre>{<br>assignee: 1,<br>text: "User One", <br>subtext: "Sales Manager", <br>avatar: "https://picsum.photos/200/300", <br>color: "#ab2d2d",<br> }</pre> |
| resourceFields | Object. Map the resources correct fields. <br>_Example_:<pre>{<br>idField: "admin_id", <br>textField: "title", <br>subTextField: "mobile",<br>avatarField: "title", <br>colorField: "background",<br>}</pre> |
| resourceHeaderComponent | Function(resource). Override header component of resource |
Expand All @@ -84,12 +83,12 @@ All props are _optional_
| translations | Object. Translations view props. <br> _default_: <pre>{<br> navigation: {<br> month: "Month",<br> week: "Week",<br> day: "Day",<br> today: "Today"<br> agenda: "Agenda"<br> },<br> form: {<br> addTitle: "Add Event",<br> editTitle: "Edit Event",<br> confirm: "Confirm",<br> delete: "Delete",<br> cancel: "Cancel"<br> },<br> event: {<br> title: "Title",<br> start: "Start",<br> end: "End",<br> allDay: "All Day"<br>},<br> validation: {<br> required: "Required",<br> invalidEmail: "Invalid Email",<br> onlyNumbers: "Only Numbers Allowed",<br> min: "Minimum {{min}} letters",<br> max: "Maximum {{max}} letters"<br> },<br> moreEvents: "More...",<br> noDataToDisplay: "No data to display",<br> loading: "Loading..."<br>}</pre> |
| onEventDrop | Function(event: DragEvent<HTMLButtonElement>, droppedOn: Date, updatedEvent: ProcessedEvent, originalEvent: ProcessedEvent). Return a promise, used to update remote data of the dropped event. Return an event to update state internally, or void if event state is managed within component |
| onEventClick | Function(event: ProcessedEvent): void. Triggered when an event item is clicked |
| onEventEdit | Function(event: ProcessedEvent): void. Triggered when an event item is being edited from the popover |
| onEventEdit | Function(event: ProcessedEvent): void. Triggered when an event item is being edited from the popover |
| onCellClick | Function(start: Date, end: Date, resourceKey?: string, resourceVal?: string | number): void. Triggered when a cell in the grid is clicked |
| onSelectedDateChange | Function(date: Date): void. Triggered when the `selectedDate` prop changes by navigation date picker or `today` button |
| onViewChange | Function(view: View, agenda?: boolean): void. Triggered when navigation view changes |
| stickyNavigation | If true, the navigation controller bar will be sticky |
| onClickMore | Function(date: Date, goToDay: Function(date: Date): void): void. Triggered when the "More..." button is clicked, it receives the date and a `goToDay` function that shows a day view for a specfic date. |
| onCellClick | Function(start: Date, end: Date, resourceKey?: string, resourceVal?: string | number): void. Triggered when a cell in the grid is clicked |
| onClickMore | Function(date: Date, goToDay: Function(date: Date): void): void. Triggered when the "More..." button is clicked, it receives the date and a `goToDay` function that shows a day view for a specfic date. |

<br>

Expand Down
2 changes: 2 additions & 0 deletions src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ function App() {
ref={calendarRef}
events={EVENTS}
// events={generateRandomEvents(200)}
editable={false}
// disableViewer
/>
);
}
Expand Down
3 changes: 2 additions & 1 deletion src/lib/components/events/EventItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ const EventItem = ({ event, multiday, hasPrev, hasNext, showdate = true }: Event
onEventClick(event);
}
}}
disabled={event.disabled}
focusRipple
disabled={disableViewer || event.disabled}
Copy link
Contributor

@slash-84 slash-84 Apr 9, 2024 •
edited

Choose a reason for hiding this comment

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

This change could lead to a regression to a fix addressed here: #338

The purpose of that PR was giving the user the ability to listen for event click without using the viewer (e.g. custom navigation)

Copy link
Owner Author

@aldabil21 aldabil21 Apr 9, 2024 •
edited

Choose a reason for hiding this comment

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

got it
can you propose a fix that an event should not be focusable if its not clickable?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion also with disableViewer=true events should be clickable (and focusable). With this property someone can use the calendar and intercept event click to attach custom logics, without opening the popover provided by this library.

Copy link
Contributor

@slash-84 slash-84 Apr 9, 2024 •
edited

Choose a reason for hiding this comment

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

Pay attention also on this line:

react-scheduler/src/lib/components/events/AgendaEventsList.tsx

Line 63 in e1b9ec9

disabled={event.disabled}

The disabling logic should remain aligned between this two components.

>
<div {...dragProps} draggable={isDraggable}>
{item}
Expand Down
8 changes: 5 additions & 3 deletions src/lib/components/events/EventItemPopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ const EventItemPopover = ({ anchorEl, event, onTriggerViewer }: Props) => {
viewerTitleComponent,
hourFormat,
translations,
disableEditor,
onEventEdit,
} = useStore();
const theme = useTheme();
Expand Down Expand Up @@ -113,8 +112,11 @@ const EventItemPopover = ({ anchorEl, event, onTriggerViewer }: Props) => {
onDelete={handleDelete}
onEdit={() => {
onTriggerViewer();
!disableEditor && triggerDialog(true, event);
onEventEdit && onEventEdit(event);
triggerDialog(true, event);

if (onEventEdit && typeof onEventEdit === "function") {
onEventEdit(event);
}
}}
/>
</div>
Expand Down
11 changes: 8 additions & 3 deletions src/lib/hooks/useCellAttributes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,24 @@ interface Props {
resourceVal: string | number;
}
export const useCellAttributes = ({ start, end, resourceKey, resourceVal }: Props) => {
const { triggerDialog, onCellClick, onDrop, currentDragged, setCurrentDragged, disableEditor } =
const { triggerDialog, onCellClick, onDrop, currentDragged, setCurrentDragged, editable } =
useStore();
const theme = useTheme();

return {
disabled: !editable,
onClick: () => {
!disableEditor &&
if (editable) {
triggerDialog(true, {
start,
end,
[resourceKey]: resourceVal,
});
onCellClick && onCellClick(start, end, resourceKey, resourceVal);
}

if (onCellClick && typeof onCellClick === "function") {
onCellClick(start, end, resourceKey, resourceVal);
}
},
onDragOver: (e: DragEvent<HTMLButtonElement>) => {
e.preventDefault();
Expand Down
6 changes: 2 additions & 4 deletions src/lib/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,6 @@ export interface SchedulerProps {
viewerTitleComponent?(event: ProcessedEvent): JSX.Element;
/** if true, the viewer popover will be disabled globally */
disableViewer?: boolean;
/** if true, the editor modal will be disabled globally */
disableEditor?: boolean;
/**Resources array to split event views with resources */
resources: DefaultRecourse[];
/**Map resources fields */
Expand Down Expand Up @@ -292,7 +290,7 @@ export interface SchedulerProps {
*/
onEventClick?(event: ProcessedEvent): void;
/**
*
* Triggered when an event item is being edited from the popover
*/
onEventEdit?(event: ProcessedEvent): void;
/**
Expand All @@ -301,7 +299,7 @@ export interface SchedulerProps {
*/
deletable?: boolean;
/**
* If event is editable, applied to all events globally, overridden by event specific editable prop
* If calendar is editable, applied to all events/cells globally, overridden by event specific editable prop
* @default true
*/
editable?: boolean;
Expand Down