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

Correct size for progress component #261

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

Conversation

steffanhalv
Copy link
Contributor

Change summary

Correct sizes

Change type

  • No review
  • Small PR
  • Big PR
  • Refactor

Closes #ISSUE_NUMBER or Part of #ISSUE_NUMBER

Copy link
Collaborator

@laurensgroeneveld laurensgroeneveld left a comment

Choose a reason for hiding this comment

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

Got a few comments!

Comment on lines -82 to +85
<BccProgress class="w-1/4 bcc-progress-sm" :value="30"/>
<BccProgress class="w-1/4 bcc-progress-md" :value="22"/>
<BccProgress class="w-1/4 bcc-progress-xs" :value="30"/>
<BccProgress class="w-1/4 bcc-progress-sm" :value="22"/>
<BccProgress class="w-1/4" :value="75"/>
<BccProgress class="w-1/4" bcc-progress-base :value="75"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the Vue library, these should be handled with the size prop instead of adding classes directly (then under the hood the Vue component adds the class, so people using just the CSS can also use that).

@@ -14,17 +14,21 @@

/* Size */
.bcc-progress-container .bcc-form-label-lg .bcc-form-label-optional {
@apply text-label;
@apply text-label-base;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class no longer exists. The typography styles were updated, which is the main reason we'll be tagging a 2.0 release soon. See this page in the docs for the new classes: https://design-library-dev.developer.bcc.no/?path=/docs/tokens-typography--docs

You need to rebase your branch on the latest main. It might be easier to start a new branch because otherwise it will also try to rebase all your older commits that were already merged into main. Or maybe drop all the old commits when rebasing (let me know if you don't know how to proceed here, happy to help as rebasing in Git can get fairly complicated).

@@ -14,17 +14,21 @@

/* Size */
.bcc-progress-container .bcc-form-label-lg .bcc-form-label-optional {
@apply text-label;
@apply text-label-base;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Additional comment here: In the Figma file it's possible to see the actual typography styles, see below.

image

Here you can see that the actual text styles are heading xs and body sm. I think we should keep our implementation as close as possible to the design, so I suggest changing those, perhaps by no longer using the form label component but implementing it directly.

@laurensgroeneveld laurensgroeneveld changed the title Feature/bcc progress Correct size for progress component Oct 10, 2023
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.

2 participants