Refactor: Update UNSTABLE_EmptyState according to design #DS-1311 #1481
Conversation
✅ Deploy Preview for spirit-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
3 similar comments
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/UNSTABLE_EmptyState.twig
Outdated
Show resolved
Hide resolved
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/UNSTABLE_EmptyState.twig
Outdated
Show resolved
Hide resolved
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/UNSTABLE_EmptyState.twig
Outdated
Show resolved
Hide resolved
packages/web/src/scss/components/UNSTABLE_EmptyState/_UNSTABLE_EmptyState.scss
Outdated
Show resolved
Hide resolved
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/stories/EmptyStateDefault.twig
Outdated
Show resolved
Hide resolved
| linkUrl="#" | ||
| title="Empty State Title" | ||
| /> | ||
| <UNSTABLE_EmptyState spacing="space-700"> |
There was a problem hiding this comment.
Ok, nice idea :-) Let's discuss this and there is no right or wrong answer.
From the point of a developer who will be using this, I can now do:
<EmptyState>
<Heading>Heading</Heading>
<Text>This is an empty state</Text>
<Link href="/">Back to homepage</Link>
</EmptyState>
// or
<EmptyState>
<EmptyStateContent
description="This is an empty state"
heading="Heading"
/>
<EmptyStateButtons
buttonPrimaryText="Back to homepage"
buttonPrimaryUrl="/"
/>
</EmptyState>
// what about the third option? are we letting it go?
<EmptyState
description="This is an empty state"
heading="Heading"
buttonPrimaryText="Back to homepage"
buttonPrimaryUrl="/"
/>I am not suggesting that we must do this right now. I am just trying to find the point where we should stop our iterations and merge this and let the developer decide about the ease of use.
There could also be another option, that also makes sense. However the naming of the subcomponent EmptyStateButtons make no sense here:
<EmptyState>
<EmptyStateContent>
<Heading>Heading</Heading>
<Text>This is an empty state</Text>
</EmptyStateContent>
<EmptyStateButtons>
<Link href="/">Back to homepage</Link>
</EmptyStateButtons>
</EmptyState>As I wrote at the beginning, there is no right or wrong, just the ease of using the API we fabricate. So maybe just let it be as it is right now and test if the usability is good enough :-)
There was a problem hiding this comment.
According to discussion it will be:
<EmptyState>
<EmptyStateSection>
<Icon name="icon-name" />
</EmptyStateSection>
<EmptyStateSection>
<Heading>Heading</Heading>
<Text>This is an empty state</Text>
</EmptyStateSection>
<EmptyStateSection>
<ActionsLayout>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
</ActionsLayout>
</EmptyStateSection>
<EmptyStateSection>
<Link href="/">Back to homepage</Link>
</EmptyStateSection>
</EmptyState>
There was a problem hiding this comment.
I think we are able to make the structure a lot simpler:
<EmptyState>
<Icon name="icon-name" />
<Heading>Heading</Heading>
<Text>This is an empty state</Text>
<ActionsLayout>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
</ActionsLayout>
<Link href="/">Back to homepage</Link>
</EmptyState>It just requires something like the following CSS:
.UNSTABLE_EmptyState {
// …
max-width: 582px; // if this is the desired limit for the component and not just the demo
justify-items: center;
}What do you think? Or did I miss anything?
There was a problem hiding this comment.
Based on the design, there is some space between the sections. That is why we use the subcomponent. Please, look at the Figma file. Maybe we missed something but the offset between the sections is different than between components themselves.
There was a problem hiding this comment.
Unfortunately, the problem seems to be in this part:
<Heading>Heading</Heading>
<Text>This is an empty state</Text>
because it should be a one block. Technically there are 4 blocks in the component:
- Image/Illustration
- Headline+Text
- Action (buttons)
- Link
and I need to ensure that they all have a gap between them.
Technically it could be:
<EmptyState>
<Icon name="icon-name" />
<div>
<Heading>Heading</Heading>
<Text>This is an empty state</Text>
</div>
<ActionsLayout>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
</ActionsLayout>
<Link href="/">Back to homepage</Link>
</EmptyState>
There was a problem hiding this comment.
@curdaj I'm totally OK with what you suggest. You would use CSS to add a gap among anything inside EmptyState, plus the syntax of the whole EmptyState composition is much simpler.
There was a problem hiding this comment.
@adamkudrna Based on discussion with @literat , we decided to keep EmptyStateSection for now. We consider it as an unstable component and may change it in the future
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/UNSTABLE_EmptyStateButtons.twig
Outdated
Show resolved
Hide resolved
curdaj
requested review from
adamkudrna,
crishpeen,
pavelklibani and
a team
as code owners
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/UNSTABLE_EmptyStateSection.twig
Outdated
Show resolved
Hide resolved
tests/e2e/demo-components-compare.spec.ts-snapshots/scrollview-chromium-linux.png
Outdated
Show resolved
Hide resolved
| linkUrl="#" | ||
| title="Empty State Title" | ||
| /> | ||
| <UNSTABLE_EmptyState spacing="space-700"> |
There was a problem hiding this comment.
I think we are able to make the structure a lot simpler:
<EmptyState>
<Icon name="icon-name" />
<Heading>Heading</Heading>
<Text>This is an empty state</Text>
<ActionsLayout>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
<ButtonLink href="/" color="primary">Back to homepage</ButtonLink>
</ActionsLayout>
<Link href="/">Back to homepage</Link>
</EmptyState>It just requires something like the following CSS:
.UNSTABLE_EmptyState {
// …
max-width: 582px; // if this is the desired limit for the component and not just the demo
justify-items: center;
}What do you think? Or did I miss anything?
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/UNSTABLE_EmptyStateButtons.twig
Outdated
Show resolved
Hide resolved
packages/web-twig/src/Resources/components/UNSTABLE_EmptyState/UNSTABLE_EmptyStateSection.twig
Show resolved
Hide resolved
packages/web/src/scss/components/UNSTABLE_EmptyState/index.html
Outdated
Show resolved
Hide resolved
7 similar comments
|
Here is the URL of the uploaded artifact: |
3 similar comments
2 similar comments
11 similar comments
curdaj
merged commit 99e3c8b
into
integration/unstable-emptystate
Description
Additional context
Issue reference