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.

Sign up for GitHub

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

Jump to bottom

Convert dashboard to functional #534

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Convert dashboard to functional #534

merged 3 commits into from
Jan 9, 2025

Conversation

mshriver
Copy link
Contributor

@mshriver mshriver commented Dec 3, 2024 •
edited
Loading

Conversion of the Dashboard component to React functional.

  1. Allows for direct use of React and router-dom hooks.
  2. Better behavior for utilizing the URL params, simpler filtering behavior.
  3. Many 1-line functions moved to callbacks within the JSX.
  4. useEffect replacing componentDidMount, with direct correlation to the relevant state or contextual dependency.
  5. Removal of ActiveDashboard from the context, now correctly implemented with state in Dashboard
  6. Addition of defaultDashboard to the context, now correctly set when project selection changes

https://issues.redhat.com/browse/IQE-3256

@mshriver mshriver force-pushed the dashboard-conversion-3256 branch from 9bfc2a2 to 6d53c1f Compare December 3, 2024 21:21
@mshriver mshriver marked this pull request as draft December 4, 2024 13:46
@mshriver mshriver force-pushed the dashboard-conversion-3256 branch 4 times, most recently from d56a7a1 to 3a2f745 Compare December 5, 2024 17:05
@mshriver mshriver changed the title DRAFT: convert dashboard to functional Convert dashboard to functional Dec 5, 2024
@mshriver mshriver force-pushed the dashboard-conversion-3256 branch 4 times, most recently from aa798b9 to 71c18fc Compare December 9, 2024 20:57
@mshriver mshriver marked this pull request as ready for review December 10, 2024 16:32
@mshriver mshriver added enhancement New feature or request frontend labels Dec 10, 2024
@mshriver mshriver force-pushed the dashboard-conversion-3256 branch from 71c18fc to 87bcc45 Compare December 10, 2024 19:21
@mshriver mshriver mentioned this pull request Dec 10, 2024
Open
1 task
@Andrewgdewar
Copy link

I would suggest getting eslint and prettier installed on your vs-code for those tools to help you simplify and catch a lot of small nit/spacing/simplifications.

@mshriver
Copy link
Contributor Author

Thanks for the review @Andrewgdewar ! I've applied most of the suggestions, and am going to look at the self-calling await for my fetches. I'm also interested in moving those HTTP calls to the react-router-dom loader hooks instead, and maybe would use the same await pattern there?

eslint is part of the pre-commit config for the repo, maybe it needs some config updates?

@mshriver mshriver mentioned this pull request Dec 12, 2024
Merged
3 tasks
@mshriver
Copy link
Contributor Author

I opened #536 to update eslint, and add some plugins.

@mshriver mshriver changed the base branch from main to feature-react-functional December 13, 2024 11:29
@mshriver mshriver force-pushed the dashboard-conversion-3256 branch 2 times, most recently from b31fefd to f3f2216 Compare January 6, 2025 13:30
@mshriver
Convert Dashboard to functional component
b9bced5 
it would be shorter to list what didn't change.

Everything changes?

useEffect replacing compnentDidMount
No more weird handling of filter values, let API filtering do it's job
Cleaned up small behaviors around button enabling

State use makes way more sense now

dashboard_id is sanely synced when direct navigation via URL
@mshriver mshriver force-pushed the dashboard-conversion-3256 branch from f3f2216 to b9bced5 Compare January 6, 2025 13:32
@mshriver mshriver self-assigned this Jan 6, 2025
@mshriver
Copy link
Contributor Author

mshriver commented Jan 6, 2025

@john-dupuy want to have some fun and tear apart my react conversion???

LightOfHeaven1994
LightOfHeaven1994 reviewed Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@LightOfHeaven1994 LightOfHeaven1994 left a comment

Choose a reason for hiding this comment

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

there are 2 things I found, other than that - looks good to me, great work!

frontend/src/dashboard.js Outdated Show resolved Hide resolved
frontend/src/dashboard.js Outdated Show resolved Hide resolved
@mshriver
Updates from review
73df709 
empty input value for db select

don't wait for promise repsonse to set modal open state
@mshriver mshriver force-pushed the dashboard-conversion-3256 branch from b9c9420 to f836ab9 Compare January 8, 2025 20:17
@mshriver mshriver force-pushed the dashboard-conversion-3256 branch from f836ab9 to 4629967 Compare January 9, 2025 12:46
@mshriver
Copy link
Contributor Author

mshriver commented Jan 9, 2025

As this is going into a feature branch and not main, I'm going to merge with the existing review feedback.

This will continue to be tested in the feature branch with #535, and the branch will be deployed to stage for scale testing.

@mshriver mshriver merged commit 326b0cc into ibutsu:feature-react-functional Jan 9, 2025
9 checks passed
@mshriver mshriver mentioned this pull request Jan 12, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andrewgdewar Andrewgdewar Andrewgdewar left review comments

@Fynardo Fynardo Awaiting requested review from Fynardo

@ColeHiggins2 ColeHiggins2 Awaiting requested review from ColeHiggins2

@LightOfHeaven1994 LightOfHeaven1994 Awaiting requested review from LightOfHeaven1994

Assignees

@mshriver mshriver

Labels
enhancement New feature or request frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mshriver @Andrewgdewar @LightOfHeaven1994