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.

Sign up for GitHub

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

Jump to bottom

feat: component check list #3511

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

feat: component check list #3511

wants to merge 35 commits into from
+114 −3

Conversation

bashirkarimi
Copy link
Contributor

@bashirkarimi bashirkarimi commented Sep 9, 2024 •
edited
Loading

No description provided.

@bashirkarimi bashirkarimi linked an issue Sep 9, 2024 that may be closed by this pull request
[component]: Check list #3438
Open
5 tasks
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Sep 9, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: e9edf80

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@swisspost/design-system-documentation Minor
@swisspost/design-system-styles Minor
@swisspost/design-system-components-angular-workspace Patch
@swisspost/design-system-components Minor
@swisspost/internet-header Patch
@swisspost/design-system-intranet-header-workspace Patch
@swisspost/design-system-nextjs-integration Patch
@swisspost/design-system-styles-primeng-workspace Patch
@swisspost/design-system-intranet-header Minor
@swisspost/design-system-styles-primeng Minor
@swisspost/design-system-components-react Minor
@swisspost/design-system-components-angular Minor
@swisspost/design-system-intranet-header-showcase Patch
@swisspost/design-system-tokens Minor
@swisspost/design-system-icons Minor
@swisspost/design-system-migrations Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@swisspost-bot
Copy link
Contributor

swisspost-bot commented Sep 9, 2024 •
edited
Loading

Related Previews

@bashirkarimi bashirkarimi marked this pull request as ready for review September 9, 2024 14:05
@bashirkarimi bashirkarimi requested a review from a team as a code owner September 9, 2024 14:06
@gfellerph gfellerph added needs: discussion There is at least one topic in the pattern descussion meeting about the issue needs: 🏓 dev roundtable To be discussed at the roundtable of esteemed developers labels Sep 9, 2024
@bashirkarimi bashirkarimi marked this pull request as draft September 10, 2024 07:31
@gfellerph gfellerph marked this pull request as ready for review September 27, 2024 09:13
gfellerph
gfellerph requested changes Sep 27, 2024
View reviewed changes
packages/styles/src/components/list.scss Outdated
&::before {
content: '';
position: absolute;
left: 0;
Copy link
Member

Choose a reason for hiding this comment

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

The check is not centrally aligned with the first line of text of the list item
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The size of icon is fixed.

packages/styles/src/components/list.scss Outdated
@@ -0,0 +1,54 @@
@use './../themes/bootstrap/core' as *;
Copy link
Member

Choose a reason for hiding this comment

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

We're going to remove bootstrap, this import can't stay here. Please replace every reference to bootstrap with internal functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is removed.

packages/styles/src/components/list.scss Outdated
background-image: url('#{icons.get-colored-svg-url("2105", "white")}'); // 'white' should be changed with variable
}

@include media-breakpoint-down(sm) {
Copy link
Member

Choose a reason for hiding this comment

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

Please replace this bootstrap mixin with it's counterpart from https://github.com/swisspost/design-system/blob/main/packages/styles/src/mixins/_media.scss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is replaced.

@bashirkarimi bashirkarimi marked this pull request as draft September 30, 2024 13:40
@bashirkarimi bashirkarimi marked this pull request as ready for review October 1, 2024 11:52
@bashirkarimi bashirkarimi marked this pull request as draft October 1, 2024 12:51
@bashirkarimi bashirkarimi marked this pull request as ready for review October 2, 2024 08:00
gfellerph
gfellerph requested changes Oct 2, 2024
View reviewed changes
Copy link
Member

@gfellerph gfellerph left a comment

Choose a reason for hiding this comment

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

On mobile, the checklist is now misaligned.

image

And in high contrast mode, the round checks disappear:
image

Can you add a rule for high contrast mode like

@include utilities.high-contrast-mode() {
    background-color: CanvasText;
    color: Canvas;
}

@bashirkarimi bashirkarimi reopened this Oct 4, 2024
@bashirkarimi
Copy link
Contributor Author

On mobile, the checklist is now misaligned.

image And in high contrast mode, the round checks disappear: image

Can you add a rule for high contrast mode like

@include utilities.high-contrast-mode() {
    background-color: CanvasText;
    color: Canvas;
}

The high contrast mode is added.
The Tokens for icon size does not change on breakpoints, currently. Therefore, the size of icon is the same for all screen sizes and it overflow to bottom on mobile.
I asked Alessio about tokens changing on breakpoints and here is his reply:

The font-size is atm device dependent, meaning it pulls different numbers from the semantical level based on what screensice people are on. Not sure if it will just stay that way or if they want to switch it up once we got figma enterprise as we then can have 40 different breakpoints compared to 4 right now. Therefore as it is now: Desktop ≥ 1024, Tablet 600-1023, Mobile <600

@oliverschuerch oliverschuerch removed their request for review October 7, 2024 07:28
gfellerph
gfellerph requested changes Oct 10, 2024
View reviewed changes
Copy link
Member

@gfellerph gfellerph left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There should be a way to always align the checkmark on the first line of content, no matter the size of the checkmark or the text. See https://stackoverflow.com/questions/32118013/align-icon-vertically-to-the-center-of-the-first-line-of-text for some ideas.

@bashirkarimi bashirkarimi requested a review from a team as a code owner October 18, 2024 13:22
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfellerph gfellerph gfellerph requested changes

@alizedebray alizedebray Awaiting requested review from alizedebray

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[component]: Check list
5 participants
@bashirkarimi @swisspost-bot @oliverschuerch @gfellerph @alizedebray