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(components): add ProgressIndicator #251

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

Conversation

seaerchin
Copy link

Problem

Design system has a progress indicator, which is not yet implemented (but is done so in forms). This PR ports and adapts the progress indicator over.

note: hover/progress states waiting for designer, will edit to use design tokens once done.

Solution

Port over ProgressIndicator from forms, edit to use design tokens, shift styling to themes file. Note that the current implementation of our ColorMode does not actually work with the useColorMode hook, as it hooks into a different Provider. the useColorMode hook uses a different provider (see here) than the ColorModeProvider (see here).

@netlify
Copy link

netlify bot commented Feb 20, 2023

Deploy Preview for objective-bell-0ffbfb canceled.

Name Link
🔨 Latest commit 5d489d3
🔍 Latest deploy log https://app.netlify.com/sites/objective-bell-0ffbfb/deploys/645e02c284fd6a0008b7a436

Copy link
Collaborator

@karrui karrui left a comment

Choose a reason for hiding this comment

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

can use colorScheme instead of dark/light mode ba. that one more flexible? maybe can discuss more. also some theming declaration changes

height: '0.5rem',
borderRadius: 'full',
backgroundColor:
colorMode === 'light'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems overkill for this. Could we just use the _dark property to denote the color in dark mode? Or could we just add a color prop since that is the only thing light or dark mode controls?

If we want dark/light mode differentiation:

activeIndicator: {
  ...
  bg: 'interaction.support.selected',
  _dark: {
    bg: 'base.content.inverse'
  }
}

else maybe just have a default color and allow users of the component to override it with the bg prop. (I'm leaning to this)

Also also prefer bg to backgroundColor prop for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

done! preferred the dark/light as per hte suggestion (never knew about _dark lmao) cos i think the default dark mode colour will be good enough most of the time. it should still allow customisation iirc

react/src/theme/components/ProgressIndicator.ts Outdated Show resolved Hide resolved
react/src/theme/theme.ts Outdated Show resolved Hide resolved
{indicators.map((_, idx) => (
<CircleIndicator
key={idx}
isActiveIndicator={idx === currActiveIdx}
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer using _active css pseudo prop instead of a property. To do this, we could set data-active instead:

import { dataAttr } from '@chakra-ui/utils' // Required since any declaration of data-* is considered truthy, and this util removes the prop on any falsey value

...

<CircleIndicator
  ...
  data-active={dataAttr(idx === currActiveIdx)}
/>

Copy link
Author

Choose a reason for hiding this comment

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

thanks for this! i think i need to be more cognizant about using pseudo states

Comment on lines 27 to 34
_focus={
isActiveIndicator
? undefined
: {
backgroundColor: 'secondary.300',
boxShadow: `0 0 0 1px var(--chakra-colors-secondary-400)`,
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if data-active is used, then we can collapse the styling here into the theme:

// This file
<Box
      __css={styles.circleIndicator} // No more other props
      onClick={onClick}
/> 

// themes/ProgressIndicator
const baseStyle = {
  ...
  _hover: {
    bg: '...'
  },
  _active: {
    _focus: {
      bg: ...
      shadow: ...
    }
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

TIL we can nest pseudo states!

shift ocnditional stuff based on UI state to be done through css; remove system wide props
@seaerchin
Copy link
Author

build failed due to _dark ._. will double check using alpha 0.11 (builds fine on alpha 0.8); worst case will just remove the prop

@karrui
Copy link
Collaborator

karrui commented Jun 18, 2024

Ill take over this PR

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