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

Convert class based to functional components #100

Merged
merged 18 commits into from
Jul 24, 2024

Conversation

Rashmil-1999
Copy link
Collaborator

This PR converts the Data Viewer and DFR3 Viewer components to Functional components from Class-based.

@Rashmil-1999 Rashmil-1999 requested review from longshuicy and a team December 5, 2022 17:58
@Rashmil-1999 Rashmil-1999 linked an issue Feb 8, 2024 that may be closed by this pull request
@longshuicy longshuicy requested a review from Vismayak February 8, 2024 22:34
@longshuicy
Copy link
Member

@Vismayak could you take a first pass review of this PR? Since your keycloak PR might need this. Thank you so much!

@Vismayak
Copy link
Member

@Rashmil-1999 I get an error when I first go the DFR3Viewer, then to any other Viewer(Data, Hazard or Semantic Viewer) and back to the DFR3FViewer. This is the console error messages I am getting. Are you also getting the same error?
image

@Rashmil-1999
Copy link
Collaborator Author

Rashmil-1999 commented Feb 14, 2024

So it seems like a side effect of using async await. if you wait like 5 seconds before jumping to other component, everything works fine otherwise it basically mutates a state even though the component was unmounted... classbased didn't cause such issues but using hooks you've got to be more careful it seems :( anyways good catch! let me see how to fix this.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

Thank you for putting so much effort in refactoring this to hooks!
All the functionality works, just some minor comments.

Do we have task for refactoring SemanticViewer, Profile, and hazardViewer? If not could you help create separate task so in the future we could distribute the work looking at your example.

src/components/DFR3Viewer.jsx Outdated Show resolved Hide resolved
src/components/DFR3Viewer.jsx Outdated Show resolved Hide resolved
src/components/DFR3Viewer.jsx Outdated Show resolved Hide resolved
src/components/DFR3Viewer.jsx Outdated Show resolved Hide resolved
src/components/DFR3Viewer.jsx Show resolved Hide resolved
src/components/DataViewer.jsx Outdated Show resolved Hide resolved
@Rashmil-1999 Rashmil-1999 self-assigned this Feb 15, 2024
@Rashmil-1999 Rashmil-1999 requested a review from longshuicy June 17, 2024 21:28
@ywkim312
Copy link
Member

The red boxed text should be aligned? If you compare with prod, you will probably see what I mean
image

@@ -0,0 +1,196 @@
import React from "react";
Copy link
Member

Choose a reason for hiding this comment

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

how do I test this page? This is */playbook url right?

@ywkim312
Copy link
Member

@Rashmil-1999 I briefly checked the code but I am not very good at react. So if you let me know what to checked in terms of functionality, I can perform that. I have deployed this to dev and checked some viewers and it looks okay

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

Code looks good.

A few thing I noticed:

  • Search seems weird. E.g. When search an item doesn't exist; I clear the search box then search again, the search action is actually never fired leaving the page stuck on empty results.
image
  • 3D fragility is not searchable and not showing up properly. e.g. search 6303c9e2cef2881193f226bd
image
  • certain fragility breaks. e.g. 6489ba3355647c1f4968bbb8
image

package-lock.json Show resolved Hide resolved
src/components/DFR3Viewer.jsx Show resolved Hide resolved
src/components/DataViewer.jsx Outdated Show resolved Hide resolved
@Rashmil-1999
Copy link
Collaborator Author

Code looks good.

A few thing I noticed:

  • Search seems weird. E.g. When search an item doesn't exist; I clear the search box then search again, the search action is actually never fired leaving the page stuck on empty results.
image * [ ] 3D fragility is not searchable and not showing up properly. e.g. search 6303c9e2cef2881193f226bd image * [ ] certain fragility breaks. e.g. 6489ba3355647c1f4968bbb8 image

For the 1st one, everything seems to be working on my end I tried garbage val, got to the empty page and cleared the search bar and tried another string, it worked. Still I replaced the deprecated onKeyPressed with onKeyDown and it should work :/ could you please try this again.

For the 2nd and the 3rd one, I fixed the error, I have no clue how it is working in production lol but it is. I have replaced the check for is3dPlot property on the curve with is3dCurve function that we have defined. With this it seems to be showing up. For the search, I can search the ID's you gave me. Could you let me know what Issues you are facing related to search?

@longshuicy
Copy link
Member

3d Fragility curves should have a different icon e.g. 6303c9e2cef2881193f226bd
image

image

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

Everything works well. Approve.

@longshuicy longshuicy requested a review from ywkim312 July 16, 2024 20:59
Copy link
Member

@navarroc navarroc left a comment

Choose a reason for hiding this comment

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

Functionality of the updated components seems to be working as expected and matches the behavior of the latest on production so I have approved this change, but I have two general comments/questions:

  1. There are a lot of warnings/errors about style in the console, should these be addressed at some point or the style/linting updated?

  2. When selecting filters (e.g. in the DFR3 viewer), should they stay selected when doing a search? Production clears them out as well when entering text and hitting search so just asking what the better behavior is since I would think filters would stay and it would search the filtered results.

@Rashmil-1999
Copy link
Collaborator Author

Functionality of the updated components seems to be working as expected and matches the behavior of the latest on production so I have approved this change, but I have two general comments/questions:

  1. There are a lot of warnings/errors about style in the console, should these be addressed at some point or the style/linting updated?
  2. When selecting filters (e.g. in the DFR3 viewer), should they stay selected when doing a search? Production clears them out as well when entering text and hitting search so just asking what the better behavior is since I would think filters would stay and it would search the filtered results.
  1. Yeah, those should be addressed but if we do plan to upgrade to MUI 5 or even 6 then we can address them all together.
  2. Yes, that was the intended behaviour. From what I remember, search is across the database for name, type, id etc. all together where as the filters were meant to be used separately. Ideally we should have search working with the filters but I don't remember why this design choice was made.

@Rashmil-1999 Rashmil-1999 merged commit 84a2ab4 into develop Jul 24, 2024
3 checks passed
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.

Convert to Functional Components
5 participants