schedule-x icon indicating copy to clipboard operation
schedule-x copied to clipboard

Config time units

Open yringler opened this issue 1 year ago • 5 comments

Checklist

Please put "X" in the below checkboxes that apply:

  • [ ] If committing a change of production code, I have tested it in different browsers (Chrome, Firefox, Safari).
  • [ x ] If committing a new feature, I have first submitted an issue (Please note: you are free to open PRs for non-issued features, but opening an issue increases your chance of a successful PR).
  • [ ] If committing a new feature, I have also written the appropriate tests for it.
  • [ x ] I have tried to build the feature in a way, that it does not cause any breaking changes for others (unless agreed upon in an issue).

Premium

  • [ ] I'm a Schedule-X premium user

This PR solves the following problem**.

Fixes #666

How to test this PR**.

For example:

  1. Feed X and Y in the events-Prop.
  2. Click Z or A.
  3. Confirm that B is displayed.

yringler avatar Sep 23 '24 21:09 yringler

Deploy request for tranquil-scone-6e5d89 rejected.

Name Link
Latest commit 1c9be7c202364f293f28a93b142943061ded0c0c

netlify[bot] avatar Sep 23 '24 21:09 netlify[bot]

@tomosterlund , this ended up being a bigger PR than I had hoped, and it doesn't seem to work. IDK if I'll have time to work on this further; any tips, or think maybe it's something you might finish off?

yringler avatar Sep 23 '24 22:09 yringler

Did you also try overriding $app.timeUnitsImpl with another implementation of the same interface?

That might save us a lot of code changes compared to now.

I currently also don't have time to work on this, but if you want we can just leave it open for now, and then I can continue at some later point when I have the time for it

tomosterlund avatar Sep 24 '24 05:09 tomosterlund

Right, I started in that direction, but I had passed in the timeunitsimpl via the config, and then I discovered the internal config, and it kind of spiraled out from there. Now I have a better idea of how config works (like, the internal vs external, signals and builders etc). If I get to this again, I'll probably still pass it into the external config, but not move it over to the internal config.

It would be nice if the calendar could reactively change between implementations - maybe if $app exposed timeunitsimpl as a signal (with a getter to get the .value to minimize code churn)? Then it would update if the config object changed?

This was my first time working with preact/signals, I'm a bit fuzzy on parts.

I currently also don't have time to work on this, but if you want we can just leave it open for now, and then I can continue at some later point when I have the time for it

Sounds good! I guess we'll see who continues on it first. The race is on 🏎️

yringler avatar Sep 24 '24 14:09 yringler

Here's a demo I put together, with a jewish calendar impl. Not 100% if it works, though - I didn't get as far as adding unit tests to it.

Edit: Here's a branch which uses a higher level library, based on the lower level library I used in the first demo. Note that it relies on yarn patch to fix typescript typings - opened a PR in that project to fix. You'll note that the code is a ton simpler, because the utils in the lib are made for date picker / calendar needs. I'm reasonably confident this is a correct timeunits implementation 😅

yringler avatar Sep 24 '24 14:09 yringler

Unfortunately I don't see driving myself this one home anytime soon. There's just too many requests for me to focus on this.

I'd gladly review it though, if you'd find the time to finish this feature.

tomosterlund avatar Nov 20 '24 17:11 tomosterlund