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

converted class based dashboard component to functional component #8724

Merged
merged 6 commits into from
Sep 20, 2023

Conversation

EraKin575
Copy link
Contributor

Notes for Reviewers

This PR fixes #8613

changed class based component to functional

Signed commits

  • Yes, I signed my commits.

Signed-off-by: EraKin575 <tejaskumar574@gmail.com>
@EraKin575 EraKin575 changed the title convert ed class based dashboard component to functional component converted class based dashboard component to functional component Sep 12, 2023
@github-actions github-actions bot added the component/ui User Interface label Sep 12, 2023
@EraKin575
Copy link
Contributor Author

@aabidsofi19 please have a look

@Ghat0tkach
Copy link
Member

@EraKin575
Let's discuss this on Meshery Dev call. Please add this as an agenda item in the meeting minutes if you would. :)

Copy link
Contributor

@Rajdip019 Rajdip019 left a comment

Choose a reason for hiding this comment

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

LGTM. Amazing work @EraKin575

@aabidsofi19
Copy link
Contributor

aabidsofi19 commented Sep 13, 2023

. @EraKin575 but this pr also some of the same issues as in #8701 (review) . take a look

@EraKin575
Copy link
Contributor Author

@Rajdip019 thanks!

@EraKin575
Copy link
Contributor Author

@aabidsofi19 a lot of functions are unused as well as some states. Is it okay to just comment them out?

@Rajdip019
Copy link
Contributor

@aabidsofi19 a lot of functions are unused as well as some states. Is it okay to just comment them out?

Yes, you can clean them up. That's the best way.

Signed-off-by: EraKin575 <tejaskumar574@gmail.com>
@sudhanshutech
Copy link
Member

@Rajdip019 @EraKin575 are all changes incorporated here?

@Rajdip019
Copy link
Contributor

@Rajdip019 @EraKin575 are all changes incorporated here?

Yes

// searchable : true,
// setCellProps : () => ({ style : { textAlign : "center" } }),
// customHeadRender : ({ index, ...column }, sortColumn) => {
// return (
Copy link
Member

Choose a reason for hiding this comment

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

why so much commented out code, are these have no use?

Copy link
Member

Choose a reason for hiding this comment

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

if not then give description why they are commented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these functions weren't used anywhere in the component. That's why I commented them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sudhanshutech these were functions that were there before I changed to functional component. I thought these functions would be used later with a newer version. So, I just commented these out and removed the pieces of code that were converted

@sudhanshutech
Copy link
Member

sudhanshutech commented Sep 16, 2023

@EraKin575 will you provide a recording after making a build that all works fine since there are lot of changes

@EraKin575
Copy link
Contributor Author

@sudhanshutech working as expected :)

simplescreenrecorder-2023-09-18_13.06.00.mp4

Signed-off-by: EraKin575 <tejaskumar574@gmail.com>
@EraKin575
Copy link
Contributor Author

@aabidsofi19 please merge this if this looks good to do so

@aabidsofi19 aabidsofi19 merged commit 6856fd2 into meshery:master Sep 20, 2023
12 of 13 checks passed
@EraKin575 EraKin575 deleted the func branch September 20, 2023 13:46
@EraKin575 EraKin575 restored the func branch September 23, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ui User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI] Change the Dashboard form class component to a functional component.
5 participants