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

Created a new visual element - progress bar #1303

Conversation

yaten2302
Copy link
Contributor

Fixes #692

About

This PR creates a new progress bar visual element to be incorporated in Taipy.

@FredLL-Avaiga
Copy link
Member

I don't understand what you're trying to do ?
We want the component to generate 1 progress.
The component needs to be integrated into taipy-gui => we need to see an entry in factory.py if we want to be able to test it
the value should then be declared as dynamic.
The comments we made for the 1st PR are still valid here (Why use useEffect + useState when you're only testing a boolean value ?)

@yaten2302
Copy link
Contributor Author

  1. @FredLL-Avaiga, sorry for the misunderstanding, actually in the previous comments of the 1st PR, there was a suggestion that if the user wants to render 2 progress bars side by side, there has to be a functionality for that as well, here's the link, that's why I added the loop in the end so that if user wants to render more than one element, then it can be done easily. If it's not required, kindly let me know, I'll remove it👍🏼
  2. Yes, I haven't added this file in the factory.py or in the index.ts. The tests about which I was talking about was the - ProgressBar.spec.tsx file.
    Actually, I'm a beginner only, so that's why it's taking time for me to implement these. I apologize for the inconvenience caused, If any changes are required in this file, kindly tell, I'll implement those. I'll also add this file in the factory.py and in index.ts as well👍🏼

@FredLL-Avaiga
Copy link
Member

No problem at all, I suppose I should be more clear.

What I'd like is that 2 progress can be rendered side by side
ie in markdown it would be declared as <|progress|><|progress|>

The declaration in index.ts and factory.py allows to manually test the component and it's interaction with taipy.
It's quite easy as you only need to declare the component in those 2 files.
Let me know if/how I can help.

@yaten2302
Copy link
Contributor Author

@FredLL-Avaiga, I've removed the progressBarCount param, since it was not required. I've also added the ProgressBar.tsx file to index.ts and factory.py. Could you please review and tell if any changes are required?

frontend/taipy-gui/src/components/Taipy/index.ts Outdated Show resolved Hide resolved
taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/ProgressBar.tsx Outdated Show resolved Hide resolved
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

getting there

frontend/taipy-gui/src/components/Taipy/ProgressBar.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/ProgressBar.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/ProgressBar.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/ProgressBar.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/ProgressBar.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/ProgressBar.tsx Outdated Show resolved Hide resolved
@FredLL-Avaiga
Copy link
Member

can you mark the discussions as resolved when they are ?

@yaten2302
Copy link
Contributor Author

Made the changes @FredLL-Avaiga , could you please review those👍

taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
@yaten2302
Copy link
Contributor Author

yaten2302 commented May 26, 2024

@FredLL-Avaiga, could you please review the changes in the factory.py. I've removed the with_default param and renamed showProgress -> showValue.

@yaten2302
Copy link
Contributor Author

@FredLL-Avaiga, could you please review the changes in my latest commit.

FredLL-Avaiga
FredLL-Avaiga previously approved these changes May 27, 2024
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

Looks good

@yaten2302
Copy link
Contributor Author

Thanks for the review, @FredLL-Avaiga . Also, do I've to add the Progress.spec.tsx file (like the tests)? I haven't added those yet.

@FredLL-Avaiga
Copy link
Member

Thanks for the review, @FredLL-Avaiga . Also, do I've to add the Progress.spec.tsx file (like the tests)? I haven't added those yet.

yes you do

@yaten2302
Copy link
Contributor Author

@FredLL-Avaiga, I've created the tests file, but I'm not able to understand that how to add the tests completely. Could you please guide that what has to be put in here, like in the getByText("")?

@FredLL-Avaiga
Copy link
Member

@FredLL-Avaiga, I've created the tests file, but I'm not able to understand that how to add the tests completely. Could you please guide that what has to be put in here, like in the getByText("")?

Jest emulates a user Interaction.
So getbytext is used to retrieve an UI element by specifying the text that the user can see.

Once you get an element you can interact with it like a user would: click, type ...

I'm not sure it's the best for your use case.

But you can at least render the component and verify that everything shows as it should depending on the properties.

I suppose you can look at the indicator component for an example

@yaten2302
Copy link
Contributor Author

progress is not described in viselements.json

Ohh, yes! I just realized that. I apologize for my mistake. I copied the viselements.json into my branch, but forgot to add the progress in it. I've pushed the changes now. The test tests/gui/builder/control/test_progress.py which was earlier failing are also passing now 👍

taipy/gui/viselements.json Outdated Show resolved Hide resolved
@yaten2302
Copy link
Contributor Author

Added the render in viselements.json👍 @FredLL-Avaiga

Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

Almost there

taipy/gui/viselements.json Outdated Show resolved Hide resolved
FredLL-Avaiga
FredLL-Avaiga previously approved these changes Jun 14, 2024
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

👍

@FredLL-Avaiga
Copy link
Member

I'll invite @FabienLelaquais to review it and if he agrees I'll merge your PR
Well done

@yaten2302
Copy link
Contributor Author

Thank you so much for your help to me complete this PR @FredLL-Avaiga 😀
Looking forward for more contributions

Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

Nice job.
A few minor changes requested in the doc, and I suspect incomplete tests...

taipy/gui/viselements.json Outdated Show resolved Hide resolved
taipy/gui/viselements.json Outdated Show resolved Hide resolved
taipy/gui/viselements.json Outdated Show resolved Hide resolved
taipy/gui/viselements.json Outdated Show resolved Hide resolved
tests/gui/control/test_progress.py Outdated Show resolved Hide resolved
tests/gui/control/test_progress.py Outdated Show resolved Hide resolved
tests/gui/control/test_progress.py Outdated Show resolved Hide resolved
tests/gui/control/test_progress.py Outdated Show resolved Hide resolved
tests/gui/control/test_progress.py Outdated Show resolved Hide resolved
@yaten2302
Copy link
Contributor Author

Hey @FabienLelaquais, ig there's some miscommunication, actually, in the above suggestions and also in the latest commit which I've made, the default value of linear param is false i.e. by default if linear is not mentioned, then the circular element will be rendered and in my latest commit, I've changed this in the viselements.json as well. I'll create one more commit to resolve all the above issues👍

@yaten2302
Copy link
Contributor Author

Now I think everything is working fine @FabienLelaquais @FredLL-Avaiga ? I've set the default value of linear param as False because in the Progress.tsx file, by default the circular element is being rendered and not the linear one. If any change is required, do LMK👍

@yaten2302
Copy link
Contributor Author

@FabienLelaquais @FredLL-Avaiga , any update on this :)

@FabienLelaquais
Copy link
Member

@yaten2302

any update on this

Sure. I had to talk with the team for opinion, and make a decision.
We will stick to what you first proposed: the default behavior for the progress element will be 'circular'.

Let me now look at the changes and hopefully approve!
Thanks

@FabienLelaquais FabienLelaquais self-requested a review June 17, 2024 16:02
Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

All the last changes seem ok to me!
Well done, and thank you!

tests/gui/builder/control/test_progress.py Show resolved Hide resolved
@yaten2302
Copy link
Contributor Author

yaten2302 commented Jun 17, 2024

Looking forward for more contributions with Taipy🎉
Thank you!
And... anytime

@FabienLelaquais FabienLelaquais self-requested a review June 17, 2024 20:04
@FabienLelaquais FabienLelaquais self-assigned this Jun 17, 2024
Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

👌

@FabienLelaquais FabienLelaquais merged commit 8a8fb36 into Avaiga:develop Jun 17, 2024
148 of 154 checks passed
@yaten2302 yaten2302 deleted the feat/#692-Add_progress_bar_visual_element branch June 18, 2024 11:25
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 progress bar/circle visual element
3 participants