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

Slider component #975

Merged
merged 2 commits into from
Apr 24, 2023
Merged

Slider component #975

merged 2 commits into from
Apr 24, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Apr 21, 2023

This PR is part of #966

This PR brings the Slider component from client to this library.

This component will be the first one fulfiling the TransitionComponent contract. These components can be passed to any other component supporting them, or used standalone.

I have also deleted the dummy FadeComponent that was created just for transitionComponent documentation purposes, and used the Slider instead.

TODO

  • We still need to decide on the final group name for TransitionComponents (as in feedback, input, layout, etc). I have provisionally used transition.
  • We also need to document the Slider specifically and TransitionComponents in general in the pattern library. -> On a follow-up PR to avoid a too big diff

Testing steps

The slider can be seen in action in the Dialog page. Once there, search for transitionComponent and there's an example using the Slider component.

@acelaya acelaya requested a review from lyzadanger April 21, 2023 09:06

import type { TransitionComponent } from '../../types';

const Slider: TransitionComponent = ({
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 would like to be able to export this as export default function Slider() {...}, but then there's no way to type it as TransitionComponent.

It would be nice to be able to do something like export default function Slider() {...} satisfies TransitionComponent 😅

{children}
</div>
);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component is brought from client verbatim.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pro tip! GitHub now has the ability to add a comment at a module level, if that's helpful (the comment box icon at the top of each file's diff):

image

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 actually knew that! 😅

},
])
);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is brought from client verbatim. Only one import was adapted.

Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

Looking great so far! I think transition is a fine name for this group of components. Presumably the human-readable section in the pattern library would be Transitions?

Do you want to take on the challenge of adding a basic pattern library page for this component (hint: you'll need to add a new "component group" to the routes typing, but I bet you're able to figure that out pretty quick)? I can help with the details once it's in place. We can tackle documenting TransitionComponent as a concept as a separate task (i.e. probably would document it on the "Using Components" page, which needs content updates anyway).

{children}
</div>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Pro tip! GitHub now has the ability to add a comment at a module level, if that's helpful (the comment box icon at the top of each file's diff):

image

Tab,
TabList,
} from '../../../../';
import { ModalDialog } from '../../../../components/feedback';
import type { ModalDialogProps } from '../../../../components/feedback/ModalDialog';
import Library from '../../Library';
import FadeComponent from '../FadeComponent';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work on cleaning up.

@acelaya
Copy link
Contributor Author

acelaya commented Apr 21, 2023

Do you want to take on the challenge of adding a basic pattern library page for this component

Sure!

@acelaya acelaya requested a review from lyzadanger April 24, 2023 07:23
@acelaya acelaya marked this pull request as ready for review April 24, 2023 07:24
@acelaya
Copy link
Contributor Author

acelaya commented Apr 24, 2023

I'm putting this PR as "ready for review", and will work on the documentation in parallel.

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #975 (e97d1e4) into main (a0b1e78) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #975   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           52        53    +1     
  Lines          671       700   +29     
  Branches       230       238    +8     
=========================================
+ Hits           671       700   +29     
Impacted Files Coverage Δ
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

@acelaya acelaya mentioned this pull request Apr 24, 2023
Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

Yay!

Follow-up if you're up to the challenge: Update the plopfile.js and add a new template for transition components, such that we can run plop to scaffold a new transition component in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's OK to only update index (not next, which is going to go away in an upcoming minor release).

@acelaya
Copy link
Contributor Author

acelaya commented Apr 24, 2023

Follow-up if you're up to the challenge: Update the plopfile.js and add a new template for transition components, such that we can run plop to scaffold a new transition component in the future.

On my way

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.

2 participants