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

feat: add label property to simple-block #1301

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

paulo-asilva
Copy link

@paulo-asilva paulo-asilva commented Dec 4, 2024

OBVIVO-2814 (Simple Block): the label property was added to the simple block, and some adjustments were also included using the Inline component to align the position in the component.

OBVIVO-2618 (Chip): including the use of the inverse token (vars.colors.neutralLow) along with some validations to add it if inverse is true. There is also a small change in the text and icon on the chip to make it readable when using inverse.

Copy link

github-actions bot commented Dec 4, 2024

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

Copy link

github-actions bot commented Dec 4, 2024

Screenshot tests report

chip / chip-multiple-selection-inverse-true-badge-2-2
chip / chip-multiple-selection-inverse-true-badge-undefined-2

Copy link

github-actions bot commented Dec 5, 2024

Size stats

master this branch diff
Total JS 12.3 MB 12.3 MB -2.69 kB
JS without icons 2.04 MB 2.04 MB -2.69 kB
Lib overhead 72.8 kB 72.8 kB 0 B
Lib overhead (gzip) 17.1 kB 17.1 kB 0 B

Copy link

github-actions bot commented Dec 5, 2024

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-c78bu2opu-flows-projects-65bb050e.vercel.app

Built with commit fa4bbc6.
This pull request is being automatically deployed with vercel-action

Copy link
Contributor

Choose a reason for hiding this comment

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

chip is broken in inverse.

I think there is a misunderstanding about the chip task.

You need to include a logic to make the chip colors change when you use active prop with onPress/href/to

For example: <Chip href="#" active /> then, the chip is colored like in specs
https://www.figma.com/design/512a5Ke4EldL0GYffCXGCJ/%F0%9F%94%B8-Chips-Specs?node-id=3876-2059&t=JVUcLR4t1jdpQr7T-4

You can check what tokens need to use in Tokens artboard

For inverse contexts, the chips are visually the same as current implementation
image

</Text2>
</Inline>
<div className={styles.rightContent}>
<Text2 regular color={vars.colors.brand}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Text2 regular color={vars.colors.brand}>
<Text2 regular color={vars.colors.textBrand}>

image

@yceballost
Copy link
Contributor

yceballost commented Dec 13, 2024

I don't really like the idea of having 1 only PR for making two different things (when one of the changes is in a core component like chips)

If we keep with this idea, I would recommend changing the PR title to include chip changes

@atabel
Copy link
Collaborator

atabel commented Dec 16, 2024

agree with @yceballost, please make 2 different PRs

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.

3 participants