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(kcheckbox): reskin component [KHCP-8973] #1792

Merged
merged 26 commits into from
Oct 27, 2023

Conversation

portikM
Copy link
Member

@portikM portikM commented Oct 25, 2023

Summary

Jira
Figma

KCheckbox component reskinning.

Links:

PR Checklist

  • Conventional Commits all commits follow the conventional commit standards outlined in the main README.
  • Tests coverage: test coverage was added for new features and bug fixes
  • Docs: includes a technically accurate README

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for kongponents-sandbox ready!

Name Link
🔨 Latest commit c382aeb
🔍 Latest deploy log https://app.netlify.com/sites/kongponents-sandbox/deploys/653be1e1cccc84000874f03f
😎 Deploy Preview https://deploy-preview-1792--kongponents-sandbox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@portikM portikM marked this pull request as draft October 25, 2023 16:56
@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for kongponents ready!

Name Link
🔨 Latest commit c382aeb
🔍 Latest deploy log https://app.netlify.com/sites/kongponents/deploys/653be1e1ccd7740008048efe
😎 Deploy Preview https://deploy-preview-1792--kongponents.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@portikM portikM force-pushed the feat/khcp-8973-kcheckbox-component-reskin branch 6 times, most recently from 74d9089 to 131047d Compare October 26, 2023 16:13
@portikM portikM force-pushed the feat/khcp-8973-kcheckbox-component-reskin branch from 131047d to 2cf0a97 Compare October 26, 2023 16:57
@portikM portikM marked this pull request as ready for review October 26, 2023 17:01
docs/components/checkbox.md Outdated Show resolved Hide resolved
docs/components/checkbox.md Outdated Show resolved Hide resolved
src/components/KCheckbox/KCheckbox.cy.ts Show resolved Hide resolved
src/components/KCheckbox/KCheckbox.vue Outdated Show resolved Hide resolved
Comment on lines 148 to 156
@mixin kCheckboxIcon {
inset: 0;
left: calc(50% - 2.4px); // 2px is not enough, 3px is too much...
pointer-events: none;
position: absolute;
top: calc(50% + 1.75px); // 1px is not enough, 2px is too much...
transform: translate(-50%, -50%);
z-index: 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: can this go in the mixins file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this mixin will be reused - it's pretty specific to this component so I would leave it here.

.checkbox-input {
@include radioCheckboxDefaults;

border-radius: var(--kui-border-radius-20, $kui-border-radius-20);
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming we're overriding the radius of the mixin above? If yes, can you please add a comment on the line above?

Copy link
Member Author

Choose a reason for hiding this comment

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

No - the mixin doesn't have the border-radius property (because it's used in both KRadio and KCheckbox - there's no default value we want for it). So here we're literally setting border-radius property (same in KRadio). I will add a comment mentioning that.

src/components/KCheckbox/KCheckbox.vue Outdated Show resolved Hide resolved
Copy link
Member

@adamdehaven adamdehaven left a comment

Choose a reason for hiding this comment

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

LGTM

@portikM portikM enabled auto-merge (squash) October 27, 2023 16:18
@portikM portikM merged commit af7c0d5 into alpha Oct 27, 2023
6 checks passed
@portikM portikM deleted the feat/khcp-8973-kcheckbox-component-reskin branch October 27, 2023 16:23
kongponents-bot pushed a commit that referenced this pull request Oct 27, 2023
# [9.0.0-alpha.31](v9.0.0-alpha.30...v9.0.0-alpha.31) (2023-10-27)

### Features

* **kcheckbox:** reskin component [KHCP-8973] ([#1792](#1792)) ([af7c0d5](af7c0d5))
@kongponents-bot
Copy link
Collaborator

🎉 This PR is included in version 9.0.0-alpha.31 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants