Skip to content

pfe-2018 Updated togglebox color in default state both color theme wh… #1028

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

PriJoh
Copy link
Collaborator

@PriJoh PriJoh commented Dec 13, 2024

Description

I’ve updated the togglebox colors in the default state for both the white and grey themes.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

image
image
image
image
image
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have updated the CHANGELOG document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Review instructions

Review instructions

Copy link

github-actions bot commented Dec 13, 2024

Size Change: +412 B (+0.04%)

Total Size: 1.17 MB

Filename Size Change
dist/designguide/styles/payex.css 53.8 kB +189 B (+0.35%)
dist/styles/payex.css 53.8 kB +189 B (+0.35%)
ℹ️ View Unchanged
Filename Size Change
dist/designguide/scripts/dg-dashboard.js 73.2 kB 0 B
dist/designguide/scripts/dg.js 24.9 kB 0 B
dist/designguide/styles/documentation-payex.css 10.2 kB +17 B (+0.17%)
dist/designguide/styles/documentation-swedbankpay.css 9.27 kB 0 B
dist/designguide/styles/swedbankpay.css 50.9 kB 0 B
dist/scripts/3563.js 72.1 kB 0 B
dist/scripts/dg-dashboard.js 73.2 kB 0 B
dist/scripts/dg.js 24.9 kB 0 B
dist/scripts/payex.js 326 kB 0 B
dist/scripts/swedbankpay.js 326 kB 0 B
dist/styles/documentation-payex.css 10.2 kB +17 B (+0.17%)
dist/styles/documentation-swedbankpay.css 9.27 kB 0 B
dist/styles/swedbankpay.css 50.9 kB 0 B

compressed-size-action

Joelhagg
Joelhagg previously approved these changes Dec 13, 2024
Copy link
Collaborator

@Joelhagg Joelhagg left a comment

Choose a reason for hiding this comment

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

Bra jobbat ✨ Snyggt med kommentarerna 👌

.showcase-panel.showcase-panel-advanced
.component-preview
.component-preview-content:has(input[type="checkbox"].theme-white) {
background-color: var(--bg-togglebox-theme-white) !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Denna !important går inte att komma ifrån va? 😜

Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is 1 single selector, with no comma, then keep it all on the same line

Copy link
Collaborator

Choose a reason for hiding this comment

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

AND if this rule is about the display of the documentation preview then it should not be specified here (since those rules will be shipped in the npm package) but instead be declared in the LESS file used for the documentation website only (which rules will not be used for the npm package)

-> "src\less\documentation-payex.less"

Comment on lines 4 to 18
.togglebox:has(input.theme-white) input[type="checkbox"]:not(:disabled) + label:after {
background-color: var(--brand-secondary);
}

.togglebox:has(input.theme-white) input[type="checkbox"]:not(:disabled) + label:before {
background-color: var(--white);
}

.togglebox:has(input.theme-white) input[type="checkbox"]:not(:disabled):checked + label:before {
background-color: var(--brand-success);
}

.togglebox:has(input.theme-white) input[type="checkbox"]:not(:disabled):checked + label:after {
background-color: var(--white);
}
Copy link
Collaborator

@goldenraphti goldenraphti Dec 13, 2024

Choose a reason for hiding this comment

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

@PriJoh you can make it easier by re-addressing the togglebox custom-properties. This way no need to going through all the selectors again.

e.g.

.togglebox:has(input.theme-white) {
    --checked-thumb-color: var(--white);
    --checked-track-color: var(--brand-success);
    --unchecked-thumb-color: var(--white);
    --unchecked-track-color: var(--brand-secondary);
}

and doing the same for the gray theme

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have looked at all the comments and they should now be resolved :)

@@ -195,6 +195,7 @@ body {
--bg-grey: var(--brand-secondary-light-2, #c8c8c8);
--bg-white: #ffffff;
--bg-quaternary: var(--list-gray, #999999);
--bg-togglebox-theme-white: #d9d9d9;
Copy link
Collaborator

@goldenraphti goldenraphti Dec 13, 2024

Choose a reason for hiding this comment

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

if this rule is only used for documentation and not meant to ever be used by projects consuming the design system then move it to the "documentation-payex.less" file. This way we keep the files shipped cleaner and lighter.

@PriJoh PriJoh requested a review from goldenraphti December 16, 2024 10:15
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