-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(Blocks): new Mistica Community Components #770
Conversation
Size stats
|
Accessibility report ℹ️ You can run this locally by executing |
Deploy preview for mistica-web ready! ✅ Preview Built with commit 0050e59. |
Screenshot tests report ✔️ All passing |
src/blocks.tsx
Outdated
}} | ||
aria-label={ariaLabel} | ||
> | ||
{(title || stackingGroup || description) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the behavior of "not having the title prop, the stackingGroup is not showing" quite odd.
https://tinyurl.com/2lalc6qz
I would appreciate the flexibility to use this component with just the title + progress, or solely with the stackingGroup + progress, for example
I plan to discuss this matter with the Vivo design team to propose a change.
Please, include a link to figma specs in the PR |
secondHeading?: { | ||
value: string; | ||
text: string; | ||
}; | ||
|
||
secondaryValue?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm.. I still see this quite confusing. Can I have secondHeading and secondaryValue at the same time in the component?
src/community/blocks.tsx
Outdated
title?: string; | ||
stackingGroup?: RendersNullableElement<typeof StackingGroup>; | ||
|
||
progressPercent?: number; | ||
reverse?: boolean; | ||
|
||
value: string; | ||
text: string; | ||
description?: string; | ||
|
||
valueColor?: string; | ||
'aria-label'?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense to have a ProgressBlock
without a ProgresBar
If figma specs say so, then figma specs must be wrong.
Why did you changed value to accept number? if it should only accept numbers, then type it as number
, if it should accept any string then type it as string
but don't type it as number | string
, please
src/community/blocks.tsx
Outdated
title?: string; | ||
stackingGroup?: RendersNullableElement<typeof StackingGroup>; | ||
|
||
progressPercent?: number; | ||
reverse?: boolean; | ||
|
||
value: string; | ||
text: string; | ||
description?: string; | ||
|
||
valueColor?: string; | ||
'aria-label'?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div aria-label={ariaLabel}> | ||
{headline && <Box paddingBottom={24}>{headline}</Box>} | ||
|
||
<Stack space={2}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this component the one to render this?
For first slot:
I assume 300 Mega => would be mainHeading.value
I assume de velocidade => would be mainHeading.text
But there's no possibility of putting a text above this mainHeading for "Vivo Fibra 300 Mbps Especial Netflix_1"
As headline is a Tag.
For second slot, similar problem:
I assume 110 canais is => mainHeading.value
But mainHeading.text is mandatory.
And there's no possibility of putting a text above this mainHeading for "Vivo Play Avançado HD"
As headline is a Tag.
The other alternative is that we are supposed to use the ProgressBlock for this but without a progress bar?
But heading.text is mandatory, so we also cannot do the case for TV.
Note: screenshot tests for the actual cases we want to use would be helpful in identifying which block to be used in each case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BrunoGuimaraes103, this is important. Can you check the definition with the designers? The blocks should be able to cover these cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are checking about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Winde Sorry, I'm kind of confused on what you mean, could you be a little clearer?
The use cases in question can be done using ProgressBlock and ValueBlock as shown in this image:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird:
(1) to have to use a Progress block without progress bar
(2) That these two cases are actually different blocks
but fair enough :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's quite weird. I said the same in other comment :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the use case for landline and TV, there's some type misalignment
Neither Highlighted nor Progress blocks seem fit to do those cases
<div aria-label={ariaLabel}> | ||
{headline && <Box paddingBottom={24}>{headline}</Box>} | ||
|
||
<Stack space={2}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's quite weird. I said the same in other comment :(
# [14.18.0](v14.17.1...v14.18.0) (2023-07-20) ### Bug Fixes * **DisplayMediaCard, PosterCard:** add label to video control ([#823](#823)) ([dc765ef](dc765ef)) * **row:** add missing dataAttributes ([#815](#815)) ([47e7cab](47e7cab)) ### Features * **AdvancedDataCard:** new community component ([#780](#780)) ([2803b9c](2803b9c)) * **Blocks:** new Mistica Community Components ([#770](#770)) ([8fc368a](8fc368a)) * **Header:** small version, allow removing vertical padding ([#822](#822)) ([448387e](448387e)) * **Nakedcard:** new component ([#819](#819)) ([688bb8e](688bb8e))
🎉 This PR is included in version 14.18.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.