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(web): Introduce UNSTABLE Toggle component #DS-1345 #1543

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

crishpeen
Copy link
Member

Description

Additional context

Issue reference

@github-actions github-actions bot added the feature New feature or request label Jul 14, 2024
Copy link

netlify bot commented Jul 14, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 8a409b8
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/66966294ab6d1a0008bce213
😎 Deploy Preview https://deploy-preview-1543--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (no change from production)
Accessibility: 93 (no change from production)
Best Practices: 100 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 14, 2024

Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 8a409b8
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/6696629412a7080008701c02

@coveralls
Copy link

coveralls commented Jul 14, 2024

Coverage Status

coverage: 77.951%. remained the same
when pulling 8a409b8 on feat/web-toggle
into bd48607 on main.

Copy link
Contributor

@dlouhak dlouhak left a comment

Choose a reason for hiding this comment

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

🎩 , I have only a few comments.

During the code review, I came up with a few insights, mostly related to the stable version of this component:

  1. In the Figma it is possible to set if the component has icon or not. This shouldn't be implemented?
image
  1. Should it be possible change state by clicking on the validation/helper text? (related to the Checkbox component, where you probably inspired yourself). And I've also noticed that in the example of validation states the danger variant with multiple error messages doesn't have cursor over label.

  2. Nitpicking idea for form components: If there would be a composition of various form components, I can imagine that designer would like to have possibility to set order of the component (toggle-label or label-toggle), because eg. Checkbox has input first and then label. The same applies for transitions between checked/unchecked states.
    image

@crishpeen
Copy link
Member Author

🎩 , I have only a few comments.

During the code review, I came up with a few insights, mostly related to the stable version of this component:

  1. In the Figma it is possible to set if the component has icon or not. This shouldn't be implemented?
image 2. Should it be possible **change state by clicking on the validation/helper text**? (related to the Checkbox component, where you probably inspired yourself). And I've also noticed that in the example of validation states the danger variant with multiple error messages doesn't have cursor over label. 3. Nitpicking idea for form components: If there would be a composition of various form components, I can imagine that designer would like to have **possibility to set order of the component** (toggle-label or label-toggle), because eg. `Checkbox` has input first and then label. The same applies for transitions between checked/unchecked states. ![image](https://private-user-images.githubusercontent.com/33354913/348665320-608cb42d-2cbb-4b92-acb0-8488baf00614.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjExMTgyMDAsIm5iZiI6MTcyMTExNzkwMCwicGF0aCI6Ii8zMzM1NDkxMy8zNDg2NjUzMjAtNjA4Y2I0MmQtMmNiYi00YjkyLWFjYjAtODQ4OGJhZjAwNjE0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MTYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzE2VDA4MTgyMFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTBhMDI0NzEwNjI2MTNkNjdmMmVmMTM2YzE2Mzg1NWIxNjgxYmEzNWZiZGQxYzA4MTAxYzEzMDQwNTgxMzI3NzQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.A7xCGwBKwHcHCNdgicO9quxCNZC656W-69eKAFT2lac)
  1. yes, I missed that
  2. it acts the same as in checkbox, the danger state with multiple validation texts is a problem and I will create a topic for our thurday talks, because currently we are causing invalid HTML and I don't know what is the best solution. The current state is as elsewhere (not ideal, but working)
  3. We decided to do this layout now. In relatively short term we plan to split all form components to small pieces and user will be able to combine them as they want.

@crishpeen crishpeen added the run-visual-tests Runs visual regression testing on this PR label Jul 16, 2024
Copy link
Contributor

@crishpeen crishpeen merged commit 100ad5e into main Jul 16, 2024
17 checks passed
@crishpeen crishpeen deleted the feat/web-toggle branch July 16, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request run-visual-tests Runs visual regression testing on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants