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(react,styles): synced styles for progress bar and added thin variant #1309

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

Conversation

orest-s
Copy link
Contributor

@orest-s orest-s commented Jan 4, 2024

Closes: #915

@orest-s orest-s requested review from a team as code owners January 4, 2024 14:21
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1309.d15792l1n26ww3.amplifyapp.com

Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

This is looking really good! Just a few minor changes needed.

packages/react/src/components/ProgressBar/index.tsx Outdated Show resolved Hide resolved
--progress-bar-animation-timing: 150ms;
}

.ProgressBar {
background-color: var(--progress-bar-background-color);
padding: 2px;
padding: var(--space-half);
Copy link
Member

Choose a reason for hiding this comment

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

This is an instance where we don't really want spacing to scale with the global padding variable. The padding in this instance is acting like a border, which we do want to have a fixed size. Additionally, I think the padding is wrong here. The thin/normal variants have a 16px and 24px footprint respectively, so this appears to be 3px from the design.

@@ -13,6 +13,14 @@ import { ProgressBar } from '@deque/cauldron-react'

## Examples

```jsx example
Copy link
Member

Choose a reason for hiding this comment

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

With multiple examples here, we probably want each example under a level 3 heading.

@@ -18,3 +20,16 @@
min-width: var(--space-small);
transition: width linear var(--progress-bar-animation-timing);
}

.ProgressBar--thin .ProgressBar--fill {
height: var(--space-smallest);
Copy link
Member

Choose a reason for hiding this comment

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

Same issue as above comment. This should be a fixed size and not inheriting from --space-smallest.

}

.cauldron--theme-dark .ProgressBar {
border: none;
Copy link
Member

Choose a reason for hiding this comment

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

This gives the dark themed progress bar a different footprint. It would be better to update --progress-bar-border-color to match the background color for dark theme.

Comment on lines 28 to 31
.cauldron--theme-dark {
--progress-bar-background-color: var(--accent-dark);
--progress-bar-fill-color: var(--accent-info-light);
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be immediately below :root.

@scurker scurker changed the title feat(style): synced styles for progress bar and added thin variant feat(react,styles): synced styles for progress bar and added thin variant Jan 4, 2024
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.

Add additional progress bar variant
2 participants