Make transition durations dynamic Part 1

[mstrofbass]

This replaces PR #2464 and makes progress on #2110

The changes here are:

1. We now have a top-level config object for the duration timings.
2. We have a helper that wraps the config with getters that return 0 instead of the duration when we're in the e2e tests.
3. The PandaCSS config is generated based on the top-level config.
4. The CSSTransition components use the helper to get the appropriate duration.

NOTE: I wanted to be able to test the helper under testing conditions so I had to add some provisions to allow for overriding the state and force it to operate as if it was in tests. We could make this a bit simpler if we brought back the isE2E helper (in it's functional form) now that we're checking this in multiple places; then isE2E could be easily mocked so we can test both paths.

[raineorshine]

Looks good! I can confirm the animations are still working.

Just one type suggestion then ready to merge.

NOTE: I wanted to be able to test the helper under testing conditions so I had to add some provisions to allow for overriding the state and force it to operate as if it was in tests. We could make this a bit simpler if we brought back the isE2E helper (in it's functional form) now that we're checking this in multiple places; then isE2E could be easily mocked so we can test both paths.

I think the benefit we get from this additional testing is probably not worth the additional complexity. It turns what is essentially an if-statement that zeros out the duration into an object-oriented class with its own interface and set of tests. The duration logic is not likely to change in the future and not likely to develop regressions.

I would generally reserve testing for 1) high-level integration testing of user behavior, or 2) low-level unit testing of nontrival functionality. Testing the test framework has diminishing returns, especially when the added test complexity exceeds the complexity of the original logic.

Maybe I'm wrong about this and the long-term benefit exceeds the additional complexity. Or perhaps I'm underestimating the possibility of regressions in the duration switch. Anyway, just wanted to share my perspective. I'm fine to leave this in since it is already done. Thanks!

[raineorshine]

@mstrofbass I resolved some merge conflicts from a PR I merged this morning, so make sure to pull the latest (dbc8719).

[mstrofbass]

Looks good! I can confirm the animations are still working.

Just one type suggestion then ready to merge.

NOTE: I wanted to be able to test the helper under testing conditions so I had to add some provisions to allow for overriding the state and force it to operate as if it was in tests. We could make this a bit simpler if we brought back the isE2E helper (in it's functional form) now that we're checking this in multiple places; then isE2E could be easily mocked so we can test both paths.

I think the benefit we get from this additional testing is probably not worth the additional complexity. It turns what is essentially an if-statement that zeros out the duration into an object-oriented class with its own interface and set of tests. The duration logic is not likely to change in the future and not likely to develop regressions.

I would generally reserve testing for 1) high-level integration testing of user behavior, or 2) low-level unit testing of nontrival functionality. Testing the test framework has diminishing returns, especially when the added test complexity exceeds the complexity of the original logic.

Maybe I'm wrong about this and the long-term benefit exceeds the additional complexity. Or perhaps I'm underestimating the possibility of regressions in the duration switch. Anyway, just wanted to share my perspective. I'm fine to leave this in since it is already done. Thanks!

It's a philosophical difference that's not a quick discussion but a few reasons I prefer it:

1. 90% of the bugs I encounter are really simple, low-level bugs that this type of testing catches but is easily missed with e2e and integration tests;
2. These really simple tests are really fast to write and have low cognitive load, so you can get broad coverage with little mental effort...integration tests require a lot more in-depth thought about how components work together so it's easier to miss things
3. e2e testing generally captures most of the stuff integration tests target, so integration tests don't really provide much extra except for certain scenarios
4. Low-level unit tests make refactoring significantly easier because it's super simple to figure out if the refactored code works just like the old;
5. For an open-source project, low level unit tests should make it easier to be confident that the contributor did proper testing because you should be able to look through

It's also widely considered a best practice...this is a bit of an appeal to authority that I don't care for but it's worth pointing out that there are a lot of justifications for it.

My general preference is full coverage via true unit tests, as full coverage as I can get via e2e tests, and integration tests for really complex portions that are difficult to test via e2e tests. Most of my testing is in backend stuff, so this paradigm gets a bit iffy in UI land but the general philosophy is still applicable.

I'm fine either way, I just default to writing low-level tests unless someone tells me not to since it's easier to write them when writing the code than it is to come back later and write them. I will also modify my style to your preferences; it will just take some probing here and there to figure out what they are!

[mstrofbass]

Will get to the requested changes later this evening!

[yangchristina]

token() is for using inside the style prop. For cva and css, you can use '...{durations.tokenName} ...' directly, and if the property only consists of '{duration.tokenName}', you can put in 'tokenName' directly.

[raineorshine]

@yangchristina Thanks for adding your PandaCSS expertise!

@mstrofbass Thank you for expanding on that! Well said.

I should definitely soften my position. Unit tests are essential for preventing regressions, and I think we should strive for as much coverage as possible. The existing unit tests have been indispensable for several major refactors, and I've never found myself wanting fewer. Integration tests should not be thought of as a replacement for unit tests, but rather as complementary. All the reasons you provided I am in deep agreement with.

To narrow it down, I guess what was most relevant here was mocking, and how it can add complexity if we're not careful. The isE2E would allow for easy mocking as you pointed out. There might also be a way to mock navigator.webdriver directly, but I don't know off the top of my head.

When mocking and/or testing the test coverage comes up again, let's discuss so we can consider the tradeoffs. Otherwise, I'm all for full test coverage, especially for src/actions, src/selectors, and src/util :).

[mstrofbass]

token() is for using inside the style prop. For cva and css, you can use '...{durations.tokenName} ...' directly, and if the property only consists of '{duration.tokenName}', you can put in 'tokenName' directly.

This is great info @yangchristina. I didn't quite get the last part there about putting tokenName directly and ran into some trouble messing around with it but I implemented your suggested changes and they seem to be working.

[mstrofbass]

To narrow it down, I guess what was most relevant here was mocking, and how it can add complexity if we're not careful. The isE2E would allow for easy mocking as you pointed out. There might also be a way to mock navigator.webdriver directly, but I don't know off the top of my head.

I tend to avoid mocking global or language stuff like that because it's frequently significantly more work than just wrapping it. I went ahead and tried to mock navigator.webdriver but got an error saying that webdriver was not defined. I was copying examples from the web so it should work since we're using jsdom but I gave up after a bit. Happy to circle back around to this after we get some of the bigger stuff knocked out.

[raineorshine]

Exactly!