Converting MesherySetting component from Class to Functional based #8701

Cvr421 · 2023-09-09T18:59:32Z

Notes for Reviewers
Converting class based component to functional based component

This PR fixes #8674

Signed commits
Yes, I signed my commits.

welcome · 2023-09-09T18:59:34Z

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, you can find updates in the #github-notifications channel in the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while performing a commit.

Cvr421 · 2023-09-09T19:01:40Z

Hii @Rajdip019 Please review this PR

Rajdip019 · 2023-09-09T19:06:57Z

ui/components/MesherySettings.js

+  // eslint-disable-next-line no-unused-vars
+  const handleError = (msg) => (error) => {
+    props.updateProgress({ showProgress : false });
+    const notify = props.notify;


use the useNotifications hook for notifications, not props. Also make sure to remove the withNotify wrapper

Rajdip019 · 2023-09-09T19:08:49Z

ui/components/MesherySettings.js

+  useEffect(() => {
+    const fetchData = async () => {
+      try {
+        const modelsResponse = await getModelsDetail();


Try to use a cleanup function and don't use await inside of a useEffect, place the fetchData function outside the useEffect.

Rajdip019 · 2023-09-09T19:11:47Z

ui/components/MesherySettings.js

+       JSON.stringify(props.meshAdapters) !== JSON.stringify(meshAdapters)
+    ) {
+
+      k8sconfig (props.k8sconfig)


What does this line and the next 4 lines do? Can you elaborate a bit?

Actually here we are checking the props.k8sconfig state conditionally that if this condition {(JSON.stringify(props.k8sconfig) !== JSON.stringify(k8sconfig)} is true then props.k8config has changed . and if it's changed then we are updating the k8sconfig local variable. this is the same logic that are mention into class based component . And same for next line

Rajdip019 · 2023-09-09T19:12:19Z

@Cvr421 make sure to change the PR name to something specific.

Also, when you do all the required changes can you attach a video of this component working before and after?

Cvr421 · 2023-09-10T07:22:36Z

@Cvr421 make sure to change the PR name to something specific.

Also, when you do all the required changes can you attach a video of this component working before and after?

Ya sure

Cvr421 · 2023-09-11T15:31:15Z

@Rajdip019 I have made the changes that are requested by you . Please review

Class based Component behaviour / Current behaviour

Classs.componene.2.mp4

Functional based component behaviour

Functional.Based.component.mp4

Rajdip019 · 2023-09-11T15:36:28Z

LGTM

sudhanshutech · 2023-09-12T07:12:40Z

@Cvr421 have you also build meshery right after the changes?

Cvr421 · 2023-09-12T08:31:26Z

@Cvr421 have you also build meshery right after the changes?

Yes @sudhanshutech i have build meshery number of time to check it is working fine .
First i run the make server on localhost 9081 and then make ui on localhost 3000

aabidsofi19 · 2023-09-13T06:39:00Z

ui/components/MesherySettings.js

+       JSON.stringify(props.meshAdapters) !== JSON.stringify(meshAdapters)
+    ) {
+
+      k8sconfig (props.k8sconfig)


what does this do , isnt k8sconfig destructured from props

@aabidsofi19
Actually here we are checking the props.k8sconfig state conditionally that if this condition {(JSON.stringify(props.k8sconfig) !== JSON.stringify(k8sconfig)} is true then props.k8config has changed . and if it's changed then we are updating the k8sconfig local variable. this is the same logic that are mention into class based component . And same for next line.

aabidsofi19 · 2023-09-13T06:39:30Z

ui/components/MesherySettings.js

+  const [scannedPrometheus, setScannedPrometheus] = useState([]);
+  const [tabVal, setTabVal] = useState(0);
+  const [subTabVal, setSubTabVal] = useState(0);
+  const [isMeshConfigured] = useState(k8sconfig.clusterConfigured);


why are we storing this in state , it is derived from props

@aabidsofi19 storing in state has multiple advantages like increase the Performance Optimization and code readability
But in functional component its recomended to use the state .
I think if you can check the class based component . this is derived form this.state which is act similar has useStates in functional component .

aabidsofi19 · 2023-09-13T06:39:50Z

ui/components/MesherySettings.js

+
+      k8sconfig (props.k8sconfig)
+      meshAdapters (props.meshAdapters)
+      grafana (props.grafana)


what does this do ?

Here we are updating the local variable k8config . Its the part of first question that you have asked above . about if
this
JSON.stringify(props.meshAdapters) !== JSON.stringify(meshAdapters)
) {
k8sconfig (props.k8sconfig)

aabidsofi19 · 2023-09-13T06:40:42Z

ui/components/MesherySettings.js

-  handleError = (msg) => (error) => {
-    this.props.updateProgress({ showProgress : false });
-    const notify = this.props.notify;
+  // eslint-disable-next-line no-unused-vars


dont disable lint rules

Actually @aabidsofi19 we are not using this handleError function anywere . so that why i am getting this error
Error: 'handleError' is assigned a value but never used . when restarting the server . that's why i disable lint rules for this function only . can we remove this function ?

Yes, you can remove the function if it's not being used anywhere. Or you could just comment that out as well marking as not being used.

Yes, you can remove the function if it's not being used anywhere. Or you could just comment that out as well marking as not being used.

ok

@Rajdip019 @aabidsofi19
I have commented the Unused function as marking as it is not in use .

aabidsofi19 · 2023-09-13T06:41:25Z

ui/components/MesherySettings.js

-    const {
-      tabVal, subTabVal, k8sconfig, meshAdapters,
-    } = this.state;
+      if (props.router.route != newRoute)props.router.push(newRoute)


router should not come from props

ok i'll update this

aabidsofi19 · 2023-09-13T06:42:03Z

ui/components/MesherySettings.js

@@ -576,5 +520,5 @@ const mapDispatchToProps = (dispatch) => ({ updateProgress : bindActionCreators(
 MesherySettings.propTypes = { classes : PropTypes.object, };

 export default withStyles(styles, { withTheme : true })(
-  connect(mapStateToProps, mapDispatchToProps)(withRouter(withNotify(MesherySettings)))
+  connect(mapStateToProps, mapDispatchToProps)(withRouter((MesherySettings)))


no need for withRouter ;use useRouter

ok i'll will update this

Cvr421 · 2023-09-13T13:49:45Z

Hii @aabidsofi19 i have updated the withRouter with useRouter .

Cvr421 · 2023-09-19T18:12:33Z

@aabidsofi19 Please review this PR . And let me know your thoughts .

Resolving git conflict Signed-off-by: Cvr421 <Chandravijayk42187@gmail.com>

Signed-off-by: Cvr421 <Chandravijayk42187@gmail.com>

Signed-off-by: Chandravijay rai <82499697+Cvr421@users.noreply.github.com>

Signed-off-by: aabidsofi19 <65964225+aabidsofi19@users.noreply.github.com>

Signed-off-by: Chandravijay rai <82499697+Cvr421@users.noreply.github.com>

Cvr421 · 2023-10-30T06:48:01Z

@aabidsofi19 Can you please review this PR and let me know your thoughts . Its been Opened from very long time .

stale · 2023-12-12T04:36:37Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2023-12-22T07:24:32Z

This issue is being automatically closed due to inactivity. However, you may choose to reopen this issue.

github-actions bot added the component/ui User Interface label Sep 9, 2023

Rajdip019 reviewed Sep 9, 2023

View reviewed changes

Cvr421 changed the title ~~Converting class to functional based component~~ Converting MesherySetting component from Class to Functional based Sep 10, 2023

Rajdip019 approved these changes Sep 11, 2023

View reviewed changes

Rajdip019 requested review from leecalcote and aabidsofi19 September 11, 2023 15:36

aabidsofi19 requested changes Sep 13, 2023

View reviewed changes

aabidsofi19 mentioned this pull request Sep 13, 2023

converted class based dashboard component to functional component #8724

Merged

1 task

Rajdip019 requested a review from aabidsofi19 September 15, 2023 11:18

Cvr421 force-pushed the functional_component branch from 91fd2aa to b007da8 Compare October 20, 2023 11:10

Cvr421 added 4 commits October 25, 2023 13:56

Converting class to functional based component

6610cee

Resolving git conflict Signed-off-by: Cvr421 <Chandravijayk42187@gmail.com>

Using UseNotification and improving useEffect

67fc947

Signed-off-by: Cvr421 <Chandravijayk42187@gmail.com>

Using useRouter instead of withRouter

bfe10d7

Signed-off-by: Cvr421 <Chandravijayk42187@gmail.com>

Commenting Unusedfuntion

dde3a3f

Signed-off-by: Cvr421 <Chandravijayk42187@gmail.com>

Cvr421 force-pushed the functional_component branch from b007da8 to dde3a3f Compare October 25, 2023 08:34

Cvr421 and others added 3 commits October 25, 2023 18:34

Merge branch 'master' into functional_component

0ecd767

Signed-off-by: Chandravijay rai <82499697+Cvr421@users.noreply.github.com>

Merge branch 'master' into functional_component

fbd5a10

Signed-off-by: aabidsofi19 <65964225+aabidsofi19@users.noreply.github.com>

Merge branch 'master' into functional_component

670b595

Signed-off-by: Chandravijay rai <82499697+Cvr421@users.noreply.github.com>

stale bot added the issue/stale Issue has not had any activity for an extended period of time label Dec 12, 2023

stale bot closed this Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converting MesherySetting component from Class to Functional based #8701

Converting MesherySetting component from Class to Functional based #8701

Cvr421 commented Sep 9, 2023

welcome bot commented Sep 9, 2023

Cvr421 commented Sep 9, 2023

Rajdip019 Sep 9, 2023 •

edited

Rajdip019 Sep 9, 2023 •

edited

Rajdip019 Sep 9, 2023

Cvr421 Sep 10, 2023 •

edited

Rajdip019 commented Sep 9, 2023

Cvr421 commented Sep 10, 2023

Cvr421 commented Sep 11, 2023

Rajdip019 commented Sep 11, 2023

sudhanshutech commented Sep 12, 2023

Cvr421 commented Sep 12, 2023

aabidsofi19 Sep 13, 2023

Cvr421 Sep 13, 2023 •

edited

aabidsofi19 Sep 13, 2023

Cvr421 Sep 13, 2023 •

edited

aabidsofi19 Sep 13, 2023

Cvr421 Sep 13, 2023

aabidsofi19 Sep 13, 2023

Cvr421 Sep 13, 2023 •

edited

Rajdip019 Sep 13, 2023

Cvr421 Sep 14, 2023

Cvr421 Sep 15, 2023

aabidsofi19 Sep 13, 2023

Cvr421 Sep 13, 2023

aabidsofi19 Sep 13, 2023

Cvr421 Sep 13, 2023

Cvr421 commented Sep 13, 2023 •

edited

Cvr421 commented Sep 19, 2023

Cvr421 commented Oct 30, 2023 •

edited

stale bot commented Dec 12, 2023

stale bot commented Dec 22, 2023

Converting MesherySetting component from Class to Functional based #8701

Converting MesherySetting component from Class to Functional based #8701

Conversation

Cvr421 commented Sep 9, 2023

welcome bot commented Sep 9, 2023

Cvr421 commented Sep 9, 2023

Rajdip019 Sep 9, 2023 • edited

Choose a reason for hiding this comment

Rajdip019 Sep 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cvr421 Sep 10, 2023 • edited

Choose a reason for hiding this comment

Rajdip019 commented Sep 9, 2023

Cvr421 commented Sep 10, 2023

Cvr421 commented Sep 11, 2023

Rajdip019 commented Sep 11, 2023

sudhanshutech commented Sep 12, 2023

Cvr421 commented Sep 12, 2023

Choose a reason for hiding this comment

Cvr421 Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cvr421 Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cvr421 Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cvr421 commented Sep 13, 2023 • edited

Cvr421 commented Sep 19, 2023

Cvr421 commented Oct 30, 2023 • edited

stale bot commented Dec 12, 2023

stale bot commented Dec 22, 2023

Rajdip019 Sep 9, 2023 •

edited

Rajdip019 Sep 9, 2023 •

edited

Cvr421 Sep 10, 2023 •

edited

Cvr421 Sep 13, 2023 •

edited

Cvr421 Sep 13, 2023 •

edited

Cvr421 Sep 13, 2023 •

edited

Cvr421 commented Sep 13, 2023 •

edited

Cvr421 commented Oct 30, 2023 •

edited