Fixes #692-Add_progress_bar_visual_element #1294
Conversation
| }, [props.linear, linear]); | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
Are you talking about the parenthesis <>?
There was a problem hiding this comment.
Yes the react.fragment (<>)
There was a problem hiding this comment.
But, without <>, I don't think that the element will be renderd? So, instead of <>, should I enclose it within the <Box>?
| return ( | ||
| <> | ||
| {value == true ? ( | ||
| <Box sx={{ width: "100%" }}> |
There was a problem hiding this comment.
I suppose we want to be able to have 2 progress bars side by side without layout
Maybe use a span instead of a Box/div ?
There was a problem hiding this comment.
Actually I'm not able to understand this completely. Like are you saying about that if the user wants to have 2 or more progress bars, for that I've to change, right?
There was a problem hiding this comment.
yes if the declaration is <|progress|><|progress|>, the user wants to see 2 progress bars side by side
There was a problem hiding this comment.
I suppose we want to be able to have 2 progress bars side by side without layout Maybe use a span instead of a Box/div ?
But, in the official doc of MUi, it's given that we've to use <Box>. Will it render the same element if we use <Span> instead?
There was a problem hiding this comment.
You don't need a <span> I would say.
Simply render the <Box> as is (without the useless fragment) and it should be ok.
You could easily test that in a React-supported Codepen, creating two of these next to each other...
There was a problem hiding this comment.
Ig my current PR is having a lot of bugs😢I'll create a new one instead and add the necessary changes👍🏼Apologies for inconvenience
There was a problem hiding this comment.
Please make sure you read and apply the comments we've added.
Thanks a lot!
There was a problem hiding this comment.
Please make sure you read and apply the comments we've added. Thanks a lot!
Sure, I'll definitely add the changes suggested here👍🏼
|
|
||
| for (let i = 0; i <= progressCount; i++) { | ||
| if (!progressVisible) { | ||
| if (linearProgress) |
There was a problem hiding this comment.
We typically force the block syntax ('{' and '}') for better readability.
This also avoids potential errors if, in the future, more directives are added to the 'true' or the 'else' part of the 'if'.
What I'm saying is:
I really prefer to see this written:
if (linearProgress) {
return (
<Box sx={{ width: "100%" }}>
<LinearProgress />
</Box>
);
} else {
return (
<Box sx={{ display: "flex" }}>
<CircularProgress />
</Box>
);
}|
|
||
| useEffect(() => { | ||
| setLinearProgress((progress) => { | ||
| if (props.linear !== undefined) return props.linear; |
There was a problem hiding this comment.
We really try to avoid this kind of shortcut:
More difficult to read, more difficult to maintain.
I would have written:
return (props.linear === undefined) ? progress : props.linear;
This applies for setProgressVisible() and setProgressCount() below.
Fixes #692
About
This PR creates a new visual element - Progress Bar (linear and circular). Currently this issue is being worked upon, this is only a draft for the review by the maintainers.