Banner Component #90
Banner Component #90
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
tests failed line 16.
error: TestingLibraryElementError: Unable to find role="button"
| ), | ||
| }; | ||
|
|
||
| export const BannerState: Story = { | ||
| ...Default, |
There was a problem hiding this comment.
Change this to ...Closeable to get the tests to pass
| position: "absolute", | ||
| top: 0, | ||
| left: 0, |
There was a problem hiding this comment.
So I think we may want to remove this default positioning for the banner. The issue is, that with absolute positioning it can cause overlap with other elements (See: here). I tested this out by placing a Banner component in the vite-react example app at the very top and you can tell that if it wasn't for the py on the Flex container, that the banner would cover the Text element that is below it. Now, the other thing to note, is that with storybook examples, once you remove this positioning, you need to make sure there is a wrapper around Banner (similar to what you did with the Stacked story):
export const Default: Story = {
render: () => (
<VStack position="absolute" inset={0}>
<Banner>Banner</Banner>
</VStack>
),
};This way you get the full "effect" you are going for with the currently positioned Banner.
There was a problem hiding this comment.
Yeah, I went back and forth on this and decided the default positioning for absolute was easier for the storybook and figured it could be modified easily with a few styles being overwrited. Another reason I used it this way is because most of our components in storybook didn't rely on any wrapper to be used and was just the component itself. Either way work just the same for me, just need to know what's preferred.
There was a problem hiding this comment.
Definitely remove positioning in the recipe, imo. I think putting a wrapper around the Banner stories is a small price to pay compared to the overlapping issues the default Banner (if absolutely positioned) would have in downstream apps. I rather opt into absolute position in a downstream app if needed, rather than having to opt out, if that makes sense.
There was a problem hiding this comment.
As a matter of fact, instead of using a wrapper in stories, you could opt into those styles:
export const Default: Story = {
render: () => <Banner position="absolute" top={0} left={0}>Banner</Banner>,
};
| /** | ||
| * Core UI banner. | ||
| */ | ||
| const Banner = ({ children, variant, closable, ...props }: Props) => { |
There was a problem hiding this comment.
change ...props to ...rest and update line 26 as well.
| export interface Props | ||
| extends Omit<ComponentProps<typeof panda.div>, "color">, | ||
| BannerVariantProps { | ||
| closable?: boolean; | ||
| } |
There was a problem hiding this comment.
Can simplify to:
export interface Props
extends ComponentProps<typeof panda.div>,
BannerVariantProps {
closable?: boolean;
}I know this was done when we were discussing color variants, but that is no long an issue (no prop collision) with your updated recipe!
| import { VStack } from "@animareflection/ui"; | ||
| import { Banner } from "@animareflection/ui/client"; | ||
|
|
||
| import { Wrapper } from "components"; | ||
|
|
||
| const BannerDemo = () => ( | ||
| <Wrapper title="Banner"> | ||
| <VStack position="relative" inset={0}> | ||
| <Banner closable>Banner</Banner> | ||
| </VStack> | ||
| </Wrapper> | ||
| ); | ||
|
|
||
| export default BannerDemo; |
There was a problem hiding this comment.
Add relative positioning to the Banner recipe (in the base), then you can simplify this:
import { Banner } from "@animareflection/ui/client";
import { Wrapper } from "components";
const BannerDemo = () => (
<Wrapper title="Banner">
<Banner closable>Banner</Banner>
</Wrapper>
);
export default BannerDemo;After that is done it all lgtm so should be able to send it!
…ry flex box in example apps
Description
Task link: https://trello.com/c/C9mrl9bu/103-banner-component
Added
Bannercomponent.Test Steps
Ensure the component's structure, stories, and testing suite are robust.
Confirm that all tests are successful.
Check that the Banner displays correctly in both storybook and sample downstream applications.