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.

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

Calendar widget #56

Merged
merged 67 commits into from
Aug 21, 2023
Merged

Calendar widget #56

merged 67 commits into from
Aug 21, 2023

Conversation

JakubSerafin
Copy link
Contributor

calendar widget that allow to show data from the table on the daily/weekly/monthly view.
features:

  • utilizing grist column mapping feature to bind any columns from table with valid type as a start / end dates, event title and "All Day" flag
  • switching between calendar perspectives (day/week/month) is persisted using grist widget options
  • linking is supported (click on the table row will navigate and highlight calendar event, clicking calendar event can be utilized by other widget to set cursor on that data)
  • drag and drop on the calendar will update data in the table
  • select time range on the calendar will create new row in the table

TESTS:
a few tests was added for CRUD operations in the table being reflected on the calendar.

…C/calendar-widget-poc

# Conflicts:
#	package.json
#	yarn.lock
…nt reference: stale element not found problem while testing binding columns to widgets
commented code removed
explanation of try-catch in SteCustomWidgetMapping
@paulfitz paulfitz self-requested a review August 11, 2023 13:21
test/getGrist.ts Outdated Show resolved Hide resolved
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

(partial review, ran out of time, will work on more later)

.github/workflows/main.yml Outdated Show resolved Hide resolved
calendar/index.html Outdated Show resolved Hide resolved
calendar/package.json Outdated Show resolved Hide resolved
calendar/package.json Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated
// when user move or resize event on the calendar, we want to update the record in the table
const onCalendarEventBeingUpdated = async (info) => {
if (info.changes?.start || info.changes?.end) {
const record = await grist.fetchSelectedRecord(info.event.id);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm why do we need to refetch the record from the application? Can't we just update the start and end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was my first approach, but TableOperations.update() seems to nullify all fields that are absent in argument.

Copy link
Member

Choose a reason for hiding this comment

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

That seems bad, is this a bug in TableOperations.update? Fetching, patching and re-saving would leave any user of this api prone to races.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it was not a bug in TableOperations.update but rather specific way in with grist.mapColumnNamesBack works. It map back all fields that was in source object, but for absent fields it just put a "undefined" under the target object value for given key.
It's up to discussion if grist.mapColumnNamesBack should work that way

Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on what grist.mapColumnNamesBack should do for absent fields @berhalak ?

calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/calendar.ts Outdated Show resolved Hide resolved
test/calendar.ts Outdated Show resolved Hide resolved
test/calendar.ts Outdated Show resolved Hide resolved
test/calendar.ts Outdated Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/getGrist.ts Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Close to being fine. I continue to find erratic whitespace distracting, most of my comments were just that, sorry.

It would probably make sense to write a widget this big in typescript rather than straight javascript, but reasonable not to tackle setting that up in the one piece of work.

calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
calendar/page.js Outdated Show resolved Hide resolved
test/calendar.ts Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/getGrist.ts Outdated Show resolved Hide resolved
test/getGrist.ts Outdated
} catch (e) {
//sometimes here we get into "detached" state and test fail.
//if this happened, just try one more time
await driver.findWait(selector, 2000).click();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await driver.findWait(selector, 2000).click();
await driver.findWait(selector, 2000).click();

Copy link
Member

Choose a reason for hiding this comment

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

Did this change turn out to be good enough for the tests to be reliable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i did some empirical tests - before in average one in ten tests run was failing on my machine on a set of 50 runs. After this change i get no single failure in 80 runs.

Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thanks @JakubSerafin !

@paulfitz paulfitz merged commit 03c9b06 into master Aug 21, 2023
1 check passed
@paulfitz paulfitz deleted the POC/calendar-widget-poc branch August 21, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants