vkurko / calendar

Full-sized drag & drop JavaScript event calendar with resource & timeline views
https://vkurko.github.io/calendar/
MIT License
969 stars 82 forks source link

Incorrect class on the main calendar wrapper #155

Closed ademaro closed 10 months ago

ademaro commented 11 months ago

The "Day" view is selected, but the "Weeks" class is not removed from the markup. err

For Month or Week view, everything works as expected.

vkurko commented 11 months ago

I will think about how to improve the css classes for different views.

vkurko commented 10 months ago

@ademaro In version 2.1.0, the logic for setting CSS classes for different views has been improved. E.g. in the timeGridDay view you should get ec-time-grid ec-day-view classes. In the timeGridWeek you'll get ec-time-grid ec-week-view and so on. Please check.

vkurko commented 10 months ago

I hope I can close this issue.

LeonHeidelbach commented 10 months ago

@vkurko I seem to have an issue which might be related to this one. After changing some theme values to create a dark mode for my app and setting the rest of the theming variables to their defaults it seems that the theme.view field does not get populated with the necessary classes to properly display the different views. As you can see in the screenshot below the classes ec-day-grid ec-month-view are missing for the dayGridMonth view to be properly displayed.

image

I did not do much with the theming but as a reference here are the values in my theme key:

theme: {
    active: 'ec-active',
    allDay: 'ec-all-day',
    bgEvent: 'ec-bg-event',
    bgEvents: 'ec-bg-events',
    body: 'ec-body',
    button: 'ec-button dark:!bg-gray-800 dark:!text-white dark:hover:!bg-gray-700 hover:cursor-pointer',
    buttonGroup: 'ec-button-group',
    calendar: 'ec',
    compact: 'ec-compact',
    content: 'ec-content',
    day: 'ec-day',
    dayFoot: 'ec-day-foot',
    dayHead: 'ec-day-head',
    daySide: 'ec-day-side',
    days: 'ec-days',
    draggable: 'ec-draggable',
    dragging: 'ec-dragging',
    event: 'ec-event',
    eventBody: 'ec-event-body',
    eventTag: 'ec-event-tag',
    eventTime: 'ec-event-time',
    eventTitle: 'ec-event-title',
    events: 'ec-events',
    extra: 'ec-extra',
    ghost: 'ec-ghost',
    handle: 'ec-handle',
    header: 'ec-header',
    hiddenScroll: 'ec-hidden-scroll',
    highlight: 'ec-highlight',
    icon: 'ec-icon dark:after:!border-white',
    line: 'ec-line',
    lines: 'ec-lines',
    noEvents: 'ec-no-events',
    nowIndicator: 'ec-now-indicator',
    otherMonth: 'ec-other-month',
    pointer: 'ec-pointer',
    popup: 'ec-popup',
    preview: 'ec-preview',
    resizer: 'ec-resizer',
    resizingX: 'ec-resizing-x',
    resizingY: 'ec-resizing-y',
    resource: 'ec-resource',
    resourceTitle: 'ec-resource-title',
    selecting: 'ec-selecting',
    sidebar: 'ec-sidebar',
    sidebarTitle: 'ec-sidebar-title',
    time: 'ec-time',
    title: 'ec-title',
    today: 'ec-today dark:!bg-gray-600 dark:!text-white',
    toolbar: 'ec-toolbar',
    uniform: 'ec-uniform',
    view: '',
    weekdays: [
        'ec-sun',
        'ec-mon',
        'ec-tue',
        'ec-wed',
        'ec-thu',
        'ec-fri',
        'ec-sat',
    ],
    withScroll: 'ec-with-scroll',
},

I also noticed after updating to the newest version 2.3.0 that when setting a function as theming callback, reloading the page works fine, but when re-rendering the component on navigating my app using sveltes goto function, on revisiting the particular page that uses the calendar component I get this error:

image

This is the callback that I set:

theme: (theme: CalendarThemeOptions) => {
    theme.button = `ec-button dark:!bg-gray-800 dark:!text-white dark:hover:!bg-gray-700
                    hover:cursor-pointer`;
    theme.icon = 'ec-icon dark:after:!border-white';
    theme.today = 'ec-today dark:!bg-gray-600 dark:!text-white';
    return theme;
},

This used to work just fine before updating but had the same issue with not properly adding the necessary theme.view classes.

vkurko commented 10 months ago

The current strategy for merging options for each veiw is as follows:

viewOptions = defaultCommonOptions < defaultViewOptions < userCommonOptions < userViewOptions

where xxxCommonOptions are those options that are intended for all views, and xxxViewOptions are respectively intended for a specific view and are passed inside the views property. < means that the options on the right overwrite the options on the left.

As you can see, by passing the theme as an object, you are essentially setting the same theme (with empty view property) for all views (I am open to suggestions to improve this mechanism).

When the theme is given as a function, instead of overwriting, this function will be called and the object from the previous merge step will be passed to it. Therefore, in your case, the option with the function should work.

Based on the error message, for some reason the theme is missing the weekdays field. I have no idea why this might be happening. Perhaps you can provide code to reproduce this error?

LeonHeidelbach commented 10 months ago

@vkurko Thank you for the very quick reply! Yes, I thought as much. When hardcoding one of the view's default css classes into the theme.view key the classes were set for every view.

In my opinion to improve the method of passing in theme overrides as an object would be to essentially just do a deep merge of the object the user sets in their options with your default values. At first I thought this was actually the way changing the theme was implemented, but after only supplying a few classes that I wanted to change, the entire calendar broke. However, if the option of supplying a function to the theme field would work for me, this would be fine as well.

A general way of improving the theme.view field in my opinion would be to just change it from being a string value into another dictionary, containing all of the different view default values. You could then just index into this object using the current view key in options.view and obtain the current class list. This way a user could just statically change the theme object without breaking the entire theming system.

I would imagine something like this:

theme : {
  view : {
    dayGridMonth : 'ec-day-grid ec-month-view',
    listDay : 'ec-list ec-day-view',
    listMonth : 'ec-list ec-month-view',
    listWeek : 'ec-list ec-week-view',
    listYear : 'ec-list ec-year-view',
    resourceTimeGridDay : 'ec-time-grid ec-resource-day-view',
    resourceTimeGridWeek : 'ec-time-grid ec-resource-week-view',
    timeGridDay : 'ec-time-grid ec-day-view',
    timeGridWeek : 'ec-time-grid ec-week-view'
  }
}

After downgrading the event calendar to version 2.2.0 the error on navigating my app disappears. However, now all css classes are just undefined.

image

This leads me to believe that the error in version 2.3.0 has nothing to do with only a missing weekdays field but rather with some entirely corrupted store variable.

This is the code I use to setup the event calendar in my +page.svelte:

<script lang="ts">
  import Calendar from '@event-calendar/core';
  import TimeGrid from '@event-calendar/time-grid';
  import Interaction from '@event-calendar/interaction';
  import DayGrid from '@event-calendar/day-grid';
  import List from '@event-calendar/list';

  let plugins = [DayGrid, TimeGrid, List, Interaction];
  let options = {
    view: 'dayGridMonth',
    headerToolbar: {
        start: 'prev,next today',
        center: 'title',
        end: 'dayGridMonth,timeGridWeek,timeGridDay,listMonth',
    },
    events: [
        {
            title: 'Test Event 1',
            start: new Date(),
            end: new Date(Date.now() + 60 * 60 * 1000),
        },
    ],
    eventClick: (evt: any) => {
        console.log(evt);
    },
    theme: (theme: CalendarThemeOptions) => {
        theme.button = `ec-button dark:!bg-gray-800 dark:!text-white dark:hover:!bg-gray-700
                        hover:cursor-pointer`;
        theme.icon = 'ec-icon dark:after:!border-white';
        theme.today = 'ec-today dark:!bg-gray-600 dark:!text-white';
        return theme;
    },
  };
</script>

<Calendar {plugins} {options} />

What I do then is simply trigger a page switch using sveltes goto function from '$app/navigation' to navigate to any other +page.svelte. When I then try to go back to the calendar page using the goto function, I get the described error. Maybe as an additional info, ssr is enabled for all pages in my app.

vkurko commented 10 months ago

I would imagine something like this:

theme : {
  view : {
    dayGridMonth : 'ec-day-grid ec-month-view',
    listDay : 'ec-list ec-day-view',
    listMonth : 'ec-list ec-month-view',
    listWeek : 'ec-list ec-week-view',
    listYear : 'ec-list ec-year-view',
    resourceTimeGridDay : 'ec-time-grid ec-resource-day-view',
    resourceTimeGridWeek : 'ec-time-grid ec-resource-week-view',
    timeGridDay : 'ec-time-grid ec-day-view',
    timeGridWeek : 'ec-time-grid ec-week-view'
  }
}

I like this idea. I wonder why I didn't think of this before :smile:

This leads me to believe that the error in version 2.3.0 has nothing to do with only a missing weekdays field but rather with some entirely corrupted store variable.

The weekdays field was added in v2.3.0. It seems that for version 2.3.0 you are passing the theme object from version 2.2.0, where this field did not exist yet.

I have not yet been able to reproduce the issue locally in SvelteKit.

LeonHeidelbach commented 10 months ago

I would imagine something like this:

theme : {
  view : {
    dayGridMonth : 'ec-day-grid ec-month-view',
    listDay : 'ec-list ec-day-view',
    listMonth : 'ec-list ec-month-view',
    listWeek : 'ec-list ec-week-view',
    listYear : 'ec-list ec-year-view',
    resourceTimeGridDay : 'ec-time-grid ec-resource-day-view',
    resourceTimeGridWeek : 'ec-time-grid ec-resource-week-view',
    timeGridDay : 'ec-time-grid ec-day-view',
    timeGridWeek : 'ec-time-grid ec-week-view'
  }
}

I like this idea. I wonder why I didn't think of this before 😄

Great, implementing this would probably already fix my issues.

This leads me to believe that the error in version 2.3.0 has nothing to do with only a missing weekdays field but rather with some entirely corrupted store variable.

The weekdays field was added in v2.3.0. It seems that for version 2.3.0 you are passing the theme object from version 2.2.0, where this field did not exist yet.

I have not yet been able to reproduce the issue locally in SvelteKit.

@vkurko This error only occurs if I set a function (i.e. the one from my previous comment) to the theme variable and not when I set a static dictionary. So there is no way that I am passing the incorrect object. ;) The weekdays field does get properly set in the theme variable that is passed to the function and so does everything else (compared it to the default by console logging). From the debug output I can see that the error occurs in the module @event-calendar/day-grid/src/Header.svelte line 11, right here:

import {
    SvelteComponentDev,
    action_destroyer,
    add_location,
    append_hydration_dev,
    attr_dev,
    children,
    claim_element,
    claim_space,
    component_subscribe, // <<= Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '0')
    destroy_each,
    detach_dev,
    dispatch_dev,
    element,
    ensure_array_like_dev,
    init,
    insert_hydration_dev,
    is_function,
    noop,
    safe_not_equal,
    space,
    validate_slots,
    validate_store
} from "/node_modules/.vite/deps/svelte_internal.js?v=3021394d";

...

// The above error points to an issue somewhere here, however there does not seem to be any more debug info
function instance($$self, $$props, $$invalidate) {
    let $theme;
    let $_days;
    let $_intlDayHeader;
    let { $$slots: slots = {}, $$scope } = $$props;
    validate_slots('Header', slots, []);
    let { theme, _intlDayHeader, _days } = getContext('state');
    validate_store(theme, 'theme');

        // The error "Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '0')" seems to point at this line
    component_subscribe($$self, theme, value => $$invalidate(0, $theme = value));
    validate_store(_intlDayHeader, '_intlDayHeader');
    component_subscribe($$self, _intlDayHeader, value => $$invalidate(2, $_intlDayHeader = value));
    validate_store(_days, '_days');
    component_subscribe($$self, _days, value => $$invalidate(1, $_days = value));
    const writable_props = [];

    Object.keys($$props).forEach(key => {
        if (!~writable_props.indexOf(key) && key.slice(0, 2) !== '$$' && key !== 'slot') console.warn(`<Header> was created with unknown prop '${key}'`);
    });

    $$self.$capture_state = () => ({
        getContext,
        setContent,
        theme,
        _intlDayHeader,
        _days,
        $theme,
        $_days,
        $_intlDayHeader
    });

    $$self.$inject_state = $$props => {
        if ('theme' in $$props) $$invalidate(3, theme = $$props.theme);
        if ('_intlDayHeader' in $$props) $$invalidate(4, _intlDayHeader = $$props._intlDayHeader);
        if ('_days' in $$props) $$invalidate(5, _days = $$props._days);
    };

    if ($$props && "$$inject" in $$props) {
        $$self.$inject_state($$props.$$inject);
    }

    return [$theme, $_days, $_intlDayHeader, theme, _intlDayHeader, _days];
}
Header.svelte:11 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '0')
    at Object.hydrate [as h] (Header.svelte:11:46)
    at Object.create [as c] (stores.js:23:1)
    at Object.create [as c] (Header.svelte:10:15)
    at create_component (Component.js:33:17)
    at Object.create [as c] (View.svelte:29:7)
    at create_component (Component.js:33:17)
    at Object.create [as c] (Calendar.svelte:137:44)
    at create_component (Component.js:33:17)
    at Object.create [as c] (+page.svelte:126:34)
    at Object.create [as c] (Content.svelte:4:43)

Could it be that you are not properly unsubscribing from a store on destroying a component? I am just handwaving here, since I am not familiar with your code base but the issue is definitely related to stores, funnily enough only if theme is set to a function though.

vkurko commented 10 months ago

I finally managed to reproduce the problem. I will fix it in the next release. Please follow #179