Enhance transition components #996

acelaya · 2023-04-26T07:43:57Z

While creating the plop files for TransitionComponents, we realized there was a design mistake on their properties. The visible one should be optional, and default to true.

However, this presented another problem, based on how we have decided to use boolean optional props. We want them to have a default value which allows to set them with no value in order to overwrite it.

If visible defaults to true, we would not be able to follow that rule, because it would have to be set as <Component visible={false} /> in order to overwrite it.

Because of this, we considered replacing the prop with something else. The first logical option was hidden, which could default to false and be overwritten with just <Component hidden />, but while implementing it, it didn't feel right, and most of the logic started with const visible = !hidden, to be able to continue dealing with visible as a concept.

After some discussions, we have moved away from a flag, and I have finally introduced an optional direction prop, which can have values in and out, with a default value of in.

The benefits of this approach are:

It matches nicely with the param provided to onTransitionEnd, making it easier to handle internally, and to reason about.
It feels right when implementing it (which didn't happen with hidden).
It removes the problem of flag props with default value.

It has one main consideration, though:

Technically speaking, it is a breaking change.

In client, where we are using Dialog and providing a local component as transitionComponent, that local component will no longer fulfill the proper API.

However, there's already a draft PR which replaces that component by the equivalent one on this library, which has been adapted as part of this PR, so the migration should be easy.

codecov · 2023-04-26T07:45:31Z

Codecov Report

Merging #996 (4ab9161) into main (7844a21) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #996   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           53        53           
  Lines          700       702    +2     
  Branches       238       240    +2     
=========================================
+ Hits           700       702    +2

Impacted Files	Coverage Δ
src/components/feedback/Dialog.tsx	`100.00% <100.00%> (ø)`
src/components/transition/Slider.tsx	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lyzadanger · 2023-04-26T13:35:13Z

src/types.ts

@@ -31,6 +31,6 @@ export type IconComponent = FunctionComponent<JSX.SVGAttributes<SVGSVGElement>>;
 * animate the mounting and unmounting of a child component.
 */
 export type TransitionComponent = FunctionComponent<{
-  visible: boolean;
+  direction?: 'in' | 'out';


Does this mean that all TransitionComponents have to support both an in and an out transition? Is this an OK assumption to make?

lyzadanger · 2023-04-26T13:48:00Z

src/pattern-library/components/patterns/UsingComponentsPage.tsx

@@ -194,7 +194,7 @@ export default function UsingComponentsPage() {
          <Library.Code
            size="sm"
            content={`type TransitionComponentProps = {
-  visible: boolean;
+  direction?: 'in' | 'out';


Can we document that default is in?

lyzadanger · 2023-04-26T13:54:30Z

src/components/feedback/Dialog.tsx

@@ -196,7 +196,7 @@ const Dialog = function Dialog({

  return (
    <Wrapper
-      visible={transitionComponentVisible}
+      direction={transitionComponentVisible ? 'in' : 'out'}


I'm curious: this Wrapper may be a reference to a TransitionComponent functional component or it might be a Fragment. Do Fragments happily take any props without grumbling? Presumably typechecking would bark if not...I just haven't thought about it before.

Yeah, I thought the same at first, but not only it does not complain when type-checking, but the IDE properly autocompletes valid props, and complains if you set something not valid for TransitionComponent.

So I guess the type of Fragment is defined in a way that it accepts anything, knowing it will be actually "lost".

lyzadanger

Cool. Let's get the tests and plop templates sorted out now!

lyzadanger · 2023-04-26T17:13:17Z

src/pattern-library/components/patterns/transition/SliderPage.tsx

-> = ({ children, visible, onTransitionEnd, _transitionStatus }) => {
-  const [isVisible, setIsVisible] = useState(visible);
-  const toggleSlider = () => setIsVisible(prev => !prev);
+> = ({ children, direction, onTransitionEnd, _transitionStatus }) => {


This typing is a little hard to read (the multi-line wrapping). Is it possible to construct the type elsewhere?

Actually, this is now redundant, and _transitionStatus can be completely discarded in favor of direction. It was needed when visible was used, but not anymore.

This can be simplified as const Slider_: TransitionComponent = ....

Ok no, my mistake. Doing that has other side effects.

I will extract the type in multiple definitions for better readability.

lyzadanger · 2023-04-26T17:14:55Z

src/pattern-library/components/patterns/transition/SliderPage.tsx

+          <Library.Example title="direction">
+            This prop tells if the Slider is currently expanded (<code>in</code>
+            ) or collapsed (<code>out</code>). It is <code>in</code> by default.


A task for me, especially now that we have reliable routing and fragment navigation, is to create a better centralized documentation for common component props. That is, this documentation probably shouldn't be necessary specifically for Slider, instead, we should be able to reuse or link to generalized documentation for this prop.

acelaya added 2 commits April 26, 2023 09:28

Change TransitionComponent API, replacing visible prop by direction

32969b1

Updated Slider docs to match current props

6cdc2f4

acelaya requested a review from lyzadanger April 26, 2023 07:43

Use direction prop in slider when calling onTransitionEnd

26942cb

acelaya force-pushed the enhance-transition-components branch from f80216d to 26942cb Compare April 26, 2023 07:50

lyzadanger reviewed Apr 26, 2023

View reviewed changes

Improve TransitionComponentProps doc with property jsdocs

dc00fb2

lyzadanger reviewed Apr 26, 2023

View reviewed changes

acelaya requested a review from lyzadanger April 26, 2023 16:13

lyzadanger approved these changes Apr 26, 2023

View reviewed changes

Extract Slider_ props to their own type in SliderPage

4ab9161

acelaya merged commit 375def9 into main Apr 27, 2023

acelaya deleted the enhance-transition-components branch April 27, 2023 07:17

acelaya mentioned this pull request Apr 27, 2023

Add support to create transition components via plop #992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance transition components #996

Enhance transition components #996

acelaya commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

lyzadanger Apr 26, 2023

lyzadanger Apr 26, 2023

acelaya Apr 26, 2023

lyzadanger Apr 26, 2023

acelaya Apr 26, 2023 •

edited

Loading

lyzadanger left a comment

lyzadanger Apr 26, 2023

acelaya Apr 27, 2023

acelaya Apr 27, 2023

lyzadanger Apr 26, 2023

Enhance transition components #996

Enhance transition components #996

Conversation

acelaya commented Apr 26, 2023 • edited Loading

codecov bot commented Apr 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

lyzadanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

acelaya Apr 26, 2023 •

edited

Loading