Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Handling calendar events with useState() hook #26

Closed
lorow opened this issue Jan 29, 2020 · 16 comments
Closed

Handling calendar events with useState() hook #26

lorow opened this issue Jan 29, 2020 · 16 comments

Comments

@lorow
Copy link

lorow commented Jan 29, 2020

Version

v1.0.5

Test Environment

Browsers - Firefox, chrome
OS - Windows

Current Behavior

My usecase requires me to update the local state of the application when user clicks on a tile.

By the documentation, clicking on a tile fires the "beforeCreateSchedule" event, so naturally I've tried collecting it (as it contains everything I need) and saving it in the local state by using the setter function returned by useState.

But doing so leads to re-rendering the page, this causes the "guide" to disappear. It also seems to strip the event of some data. While, that stripped data isn't essential for me, the disappearance of the guide isn't what I want.

Here's some code to demonstrate what I'm doing since there might be something wrong in my method:

....
const [selectedTile, setSelectedTile] = useState();
....

<Calendar
    {....calendarOptions}
    onBeforeCreateSchedule={(e) => { setSelectedTile(e) }};
/>

Expected Behavior

What I want to achieve is :

  1. The guide stays where it's been when user clicked the tile
  2. the selectedTile state has at least some basic info about the tile, for example, the date it represents
    as it's needed later down the road

Thank you in advance!

@jungeun-cho
Copy link
Contributor

Can I get a link to reproduce the problem?

For example, codesandbox, jsfiddle, codepen, etc.

Like the example below.
https://codesandbox.io/s/toast-uireact-calendar-82fi9

@lorow
Copy link
Author

lorow commented Jan 31, 2020

Yes, of course!

https://codesandbox.io/s/that-problematic-calendar-q06wn-q06wn
Here's the most striped down version.

What's more, given your example I've tinkered a little bit with my own code and I think I have managed to pin-point the problematic part.
From what I've noticed, everything works fine and nothing get's lost as long as I don't add this:

month={{ daynames: ["Niedz", "Pon", "Wto", "Śro", "Czw", "Pt", "Sb"], startDayOfWeek: 0, narrowWeekend: false }}

to the component

And it's not even that what's inside that "month" option which causes the guides to disappear, it's that option itself what does it.

Am I doing something wrong here?

@jungeun-cho
Copy link
Contributor

There was a problem with the latest release.

If you pass a month or week option when creating a calendar instance, the default operation may not be performed.
This has been fixed here.

The patch release(v1.12.11) will be tomorrow.

@lorow
Copy link
Author

lorow commented Feb 3, 2020

Makes sense, thank you!

@jungeun-cho
Copy link
Contributor

@lorow
Patched in TOAST UI Calendar v1.12.11

@coolgsus
Copy link

This still fails, at least to set week={{startDayOfWeek: 2}} with react-calendar...
https://codesandbox.io/s/toast-ui-react-calendar-forked-gtyjp?file=/src/index.tsx:3959-3994

@adhrinae
Copy link
Contributor

@coolgsus I don't recognize what's failing from the example.
Could you describe it in detail?

@coolgsus
Copy link

Yes, the calendar refreshes when you select an event (i.e., "2:6 Study").
This is when you set week to react-calendar (week={{ startDayOfWeek: 2 }}) and you use "useState" in "onClickSchedule" callback. If you don't set week in react-calendar, "useState" works without refreshing.

@adhrinae
Copy link
Contributor

@coolgsus
When you set a new state with useState, the component will invoke re-render. which means running the function (component) again.

Then the Calendar component will receive new props even though you don't recognize them. Especially it's easy not to notice for object literals and arrow functions.

The problem is that the Calendar component's diff comparing logic is so simple that can't distinguish the situation when not to re-render itself.

optionProps.forEach((key) => {
if (this.props[key] !== nextProps[key]) {
this.setOptions(key, nextProps[key]);
}
});

As workarounds, I would like to suggest a few ways though it's not good enough.

If the week/month options are not going to change, declare them outside of the component. (Recommended)

const weekOption = {
  // ...
}

function Component() {
  // ...
  return (
    <Calendar
      // ...
      week={weekOption}
      // ...
    />
  )
}

Or wrap values to useMemo.

function Component() {
  const weekOption = useMemo(() => ({
    // option values
  }), [])

  // ...

  return (
    <Calendar
      // ...
      week={weekOption}
      // ...
    />
  )
}

It won't work if you want to change option object values during events, but it will resolve the current problem.

@coolgsus
Copy link

coolgsus commented Aug 27, 2021

It worked, both ways. Thanks

@benkovy
Copy link

benkovy commented Jan 21, 2022

I'd love to open this back up and look at fixing the underlying issues.

I did a quick look and it seems that we are not doing a deep equality check that results in setting some of the properties/options even when they have not changed.

componentDidMount()

componentDidMount() {
  const {schedules = [], view} = this.props;

  this.calendarInst = new TuiCalendar(this.rootEl.current, {
    ...this.props,
    defaultView: view
  });

  this.setSchedules(this.cloneData(schedules)); // <<< changed to clone the schedules - this is an incomplete solution
  this.bindEventHandlers(this.props);
}

In component did mount we are setting the schedules. Within the implementation of Calendar.createSchedules we are setting the schedules color properties to match the colors of the calendar they belong to. This is mutating the actual schedule value and this leads to a negative affect down the line when we check for whether we should update the component.

Cloning the schedules fixes the mutation of the prop values but breaks the coloring 😢

componentShouldUpdate()

shouldComponentUpdate(nextProps) {
  const {calendars, height, schedules, theme, view} = this.props;

  if (height !== nextProps.height) {
    this.getRootElement().style.height = height;
  }

  if (JSON.stringify(calendars) !== JSON.stringify(nextProps.calendars)) {
    this.setCalendars(nextProps.calendars);
  }

  if (JSON.stringify(schedules) !== JSON.stringify(nextProps.schedules)) {
    this.calendarInst.clear();
    this.setSchedules(nextProps.schedules);
  }

  if (theme !== nextProps.theme) {
    this.calendarInst.setTheme(this.cloneData(nextProps.theme));
  }

  if (view !== nextProps.view) {
    this.calendarInst.changeView(nextProps.view);
  }

  optionProps.forEach((key) => {
    if (JSON.stringify(this.props[key]) !== JSON.stringify(nextProps[key])) {
      this.setOptions(key, nextProps[key]);
    }
  });

  this.bindEventHandlers(nextProps, this.props);

  return false;
}

The main issue here is to do with equality. we can check for shape since that is all I think we care about hear - so strigifying all of the object type values will give us that deep equality check. Now, JSON.stringify might not be the best course of action here but it does the trick - except when order changes.

I ran into the problem here where I found that schedules from props.schedules were being mutated and so the nextProps.schedules would never === props.schedules.

Let me know what you think @adhrinae is there a path forward here? How can we preserve the colors while also doing an appropriate deep equality check?

@adhrinae adhrinae reopened this Jan 24, 2022
@adhrinae
Copy link
Contributor

@benkovy

I ran into the problem here where I found that schedules from props.schedules were being mutated and so the nextProps.schedules would never === props.schedules.

I'm not sure what kind of mutations were mutated as you mentioned. Could you give me an example?


Perhaps you might notice that the component doesn't do much related to React. If some props are changed and need to be updated, the component calls instance methods of the TuiCalendar.

So I'm thinking of bringing some libraries like react-fast-compare into the prop comparison logic. It will handle Date objects and the change of order which leads to an incomplete equality check.

I'll investigate the problem and update the progress here.

@benkovy
Copy link

benkovy commented Jan 24, 2022

@adhrinae sorry yes I should have elaborated. When I pass a schedule in to the Calendar component through its schedules prop - on mount we call this.setSchedules(props.schedules).

If those schedules look like below (i.e. no color properties color, bgColor, borderColor, dragBgColor)

[
      {
        id: '1',
        calendarId: '0',
        title: 'TOAST UI Calendar Study',
        category: 'time',
        dueDateClass: '',
        start: today.toISOString(),
        end: getDate('hours', today, 3, '+').toISOString()
      }
]

Then in the internals of tui.calendar we set the colors when we create the schedules by calling setScheduleColor.

This adds the color properties to each schedule in props.schedules - which in turn means that when the component runs through shouldComponentUpdate the nextProps.schedules will be schedules that have not had their colors updated/added and so they will not be "equal" the props.schedules even if the schedule props have not changed on the surface.

In my first example (in the other comment) I cloned the schedules that are passed into the setSchedules call in the mount function. This prevents us from mutating the props but in turn causes our schedules to not have the proper colors when rendered.

@adhrinae
Copy link
Contributor

@benkovy

I see. As you noticed, it's not necessary to set color values for schedules unless you want to set specific colors to a particular schedule. I assume that the existing example is wrong.

So I just created the PR for the fix. I hope you can check the newer example and documentation makes sense to you.

#60

@adhrinae
Copy link
Contributor

adhrinae commented Jan 26, 2022

@benkovy

This adds the color properties to each schedule in props.schedules - which in turn means that when the component runs through shouldComponentUpdate the nextProps.schedules will be schedules that have not had their colors updated/added and so they will not be "equal" the props.schedules even if the schedule props have not changed on the surface.

I finally got the sense from what you mentioned. I couldn't compare schedules props during testing. So I decided to clone the schedules value when calling this.setSchedules. I guess it won't be a problem when I just clone the value using this.cloneData.

https://github.com/nhn/toast-ui.react-calendar/pull/60/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R102-R104

I think Schedule objects have to be cloned inside the core instance(which is tui-calendar itself), but it might bring the breaking change. and I'd rather focus on the next major upgrade currently I'm working on than fix this.

@adhrinae
Copy link
Contributor

Closing since the fixed version is released.

https://github.com/nhn/toast-ui.react-calendar/releases/tag/v1.0.6

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants