fix: progressbar component #27
Conversation
|
|
||
| export interface ProgressBarProps { | ||
| color?: Color; | ||
| max: number; |
There was a problem hiding this comment.
What if I dont want to pass max ? can we make it optional ?
There was a problem hiding this comment.
if we dont provide how can we determine length
There was a problem hiding this comment.
Did you not provide a default for it already ?
There was a problem hiding this comment.
Yes, that what I mean.... we can make it optional !
| export interface ProgressBarProps { | ||
| color?: Color; | ||
| max: number; | ||
| progressBarSize?: Size; |
There was a problem hiding this comment.
lets make it simple, size !!
| <ProgressBar {...args} /> | ||
| ); | ||
|
|
||
| export const Indeterminate = indeterminateProgressBarTemplate.bind({}); |
There was a problem hiding this comment.
How does this Indeterminate work ? what is the diff ?
There was a problem hiding this comment.
yes, but from a component API point of view, what props do we need for this
There was a problem hiding this comment.
if we don't provide a value prop it keeps on loading till it receives a value prop
| @@ -34,12 +35,11 @@ export default { | |||
|
|
|||
| const defaultNotificationTemplate: Story<NotificationProps> = args => ( | |||
| <Notification {...args}> | |||
| {/* replace p with text component after all merge */} | |||
| <p> | |||
| <Text> | |||
There was a problem hiding this comment.
Nice one ! Can you also check if we can add various classes to this text component ?
Check the usage of p tag in Column and Grid components !
There was a problem hiding this comment.
where and when ? and how about those classes for p tag ?
There was a problem hiding this comment.
I haven't done any changes in the extension component
There was a problem hiding this comment.
will do that later in different PR
There was a problem hiding this comment.
I am talking about this in Grid story
<Col key={i} {...args}>
<p
className={colContentClasses}
style={{ padding: '16px 0px', textAlign: 'center' }}>
<code>{col + ++i}</code>
</p>
</Col>
There was a problem hiding this comment.
oh but our text does accept any className props let me add that
There was a problem hiding this comment.
Nice one ! Can you also check if we can add various classes to this text component ?
Check the usage ofptag in Column and Grid components !
That's what I meant in this comment !! @fayez-baig
There was a problem hiding this comment.
keep it for future, let's develop components now ! this refactoring we have to do anyway in the future ! you see.... the discussion has diverted from progress bar component !. That;s why I hate adding multiple stuff in a single PR !
| @@ -21,10 +22,11 @@ const defaultBoxTemplate: Story<BoxProps> = args => ( | |||
| <article className={getStyles(['media'])}> | |||
| <div className={getStyles(['media-left'])}> | |||
| <figure className={getStyles(['image is-64x64'])}> | |||
| <img | |||
| <Image | |||
|
|
okay |
|
okay, from next time onwards I will raise a separate PR for changes |
✅ |
you may need to test every component now as you have changed so many little stuff in majority of the components ! @fayez-baig @rifaq-ahmer |
There was a problem hiding this comment.
I approve this. but next time please dont make a Bhel Puri stuff in the PR. @fayez-baig
|
yes I have told @rifaq-ahmer to check all the props |
|
okay I will try to keep it clean |
check thoroughly and deploy storybook |
|
I dont see chlidren prop working in |
|
lets discuss separetly.. not in this merged PR ! |
|
🎉 This PR is included in version 1.0.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.