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

DEV - Remove Pins from React Redux State #1865

Open
3 of 5 tasks
Tracked by #1890 ...
traycn opened this issue Nov 21, 2024 · 13 comments · May be fixed by #1920
Open
3 of 5 tasks
Tracked by #1890 ...

DEV - Remove Pins from React Redux State #1865

traycn opened this issue Nov 21, 2024 · 13 comments · May be fixed by #1920
Assignees
Labels
Milestone

Comments

@traycn
Copy link
Member

traycn commented Nov 21, 2024

Overview

We need to relegate storage of SR request data to Mapbox, and remove it from React Redux, since it is redundant and does not serve any functional purpose

Action Items

Fix non-performant loop in MapContainer (LOC 114)

  • Modify componentDidUpdate to remove comparison of pins in props. Instead, check to see if the selected NC has changed

Remove Pins from Redux

  • Remove GET_PINS_SUCCESS and GET_PINS_FAILURE from the Data reducer
    • this requires removing getPinsSuccess and getPinsFailure from our Redux sagas
    • are we using the Sagas for anything? Nobody seems to know

Optional

  • record performance print-statements (console logs), see performance ticket below

Resources

I had documented load times under various filter criteria on this ticket. We'll want to know if removing pins from Redux state will improve performance. See measurements in comments here:

@ryanfchase
Copy link
Member

ryanfchase commented Jan 11, 2025

@traycn changing title:

  • From: Fix application crashing after user submits multiple queries / large queries
  • To: Remove Non-Performant Loop in Map Lifecycle Method

I think "fixing application crashing" may be addressed by #1890

@ryanfchase ryanfchase changed the title Fix application crashing after user submits multiple queries / large queries Remove Un-Performant Loop in Map Lifecycle Method Jan 11, 2025
@ryanfchase ryanfchase changed the title Remove Un-Performant Loop in Map Lifecycle Method Remove Non-Performant Loop in Map Lifecycle Method Jan 11, 2025
@ryanfchase ryanfchase changed the title Remove Non-Performant Loop in Map Lifecycle Method Remove Pins from React Redux State Jan 14, 2025
@ryanfchase
Copy link
Member

Changing title again. We can go further from just removing the non-performant loop and simply remove pins from redux completely.

@ryanfchase ryanfchase removed the draft label Jan 14, 2025
@ryanfchase ryanfchase moved this from New Issue Approval to Prioritized Backlog in P: 311: Project Board Jan 14, 2025
@traycn traycn assigned traycn and efrenmarin45 and unassigned traycn Jan 15, 2025
@traycn traycn moved this from Prioritized Backlog to In progress in P: 311: Project Board Jan 15, 2025
@traycn traycn changed the title Remove Pins from React Redux State DEV - Remove Pins from React Redux State Jan 15, 2025
@traycn
Copy link
Member Author

traycn commented Jan 30, 2025

Hey @efrenmarin45 !

Please leave a comment with the following items:

  • updated ETA
  • progress from last week
  • Any blockers this week?
  • availability for communications during the week

@efrenmarin45
Copy link
Member

ETA Blockers Availability
2/9 latest None Monday, Wednesday and Friday this week

I would like to touch base with you @traycn when you get the chance about what I'm coming across and get your thoughts on my proposed solution before this weeks meeting if possible.

@ryanfchase ryanfchase moved this from In progress to Questions in P: 311: Project Board Feb 3, 2025
@ryanfchase ryanfchase added Question Further information is requested ready for dev lead ready for developer lead to review the issue labels Feb 3, 2025
@ryanfchase
Copy link
Member

ryanfchase commented Feb 4, 2025

Ticket Check In w/ leads about #1865

4:30pm - 5:30pm

Notes

  • Turns out that saga is completely integral to how we do state management. Previously I thought sagas were unrelated, but that's because we hadn't properly done the research around react, redux and sagas.
  • mapDispatchToProps()
    • components > Map > index.jsx, ~L500
  • getXYZ()
    • redux > reducers > data.js

Action Items

  • dev lead to create ER on turning moving to functional components

Resources

@efrenmarin45
Copy link
Member

ETA 2/10

@ryanfchase
Copy link
Member

This ticket appears to be unblocked. Moving back to in-progress.

@ryanfchase ryanfchase removed Question Further information is requested ready for dev lead ready for developer lead to review the issue labels Feb 10, 2025
@ryanfchase ryanfchase moved this from Questions to In progress in P: 311: Project Board Feb 10, 2025
@efrenmarin45
Copy link
Member

efrenmarin45 commented Feb 11, 2025

@ryanfchase @traycn Replaced the pin check (which was not initializing) with a neighborhood council ID so the component will render when the NC has changed. I also tested the removal of the pin check and Sagas involving GET_PINS_SUCCESS and GET_PINS_FAILURE and posted the results in #1716 for review. Based on our discussion, at the moment, Sagas are deeply intertwined with how the application handles state with Redux. The gains from removing the pins saga is negligible compared to the complexity it would take to unravel the relationship between Redux and state management.

I did see that some of the sagas are not being used, GET_PIN_INFO_REQUEST, for example. This might be a good addition to the overall clean up of the codebase and not necessarily a performance priority before the MVP launch, imo.

@efrenmarin45 efrenmarin45 moved this from In progress to In Review in P: 311: Project Board Feb 11, 2025
@ryanfchase
Copy link
Member

IIRC this ticket needs more work before it gets review, due to the prettier issue, yes? Moving back to In-Progress.

  • You had the right idea by moving it to the review column, but please also add ready for dev lead once it needs dev lead's attention, thank you!

@ryanfchase ryanfchase moved this from In Review to In progress in P: 311: Project Board Feb 12, 2025
@ryanfchase
Copy link
Member

Hi @efrenmarin45, thank you for your update above.

Please leave a comment with the following items:

  • updated ETA
  • progress from the last week
  • availability for communications during the week

@ExperimentsInHonesty
Copy link
Member

@efrenmarin45

Hi Efren, this is the template I want teams to use for their weekly updates. You can always add more info to the items below 1-4, which are the required minimum.

Instructions
  1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
  2. Blockers: "Difficulties or errors encountered."
  3. Availability: "How much time will you have this week to work on this issue?"
  4. ETA: "When do you expect this issue to be completed?"
  5. Pictures (if necessary): "Add any pictures that will help illustrate what you are working on."
  1. Progress:
  2. Blockers:
  3. Availability:
  4. ETA:
  5. Pictures (if necessary):

@efrenmarin45
Copy link
Member

efrenmarin45 commented Feb 16, 2025

Progress: Need to revert changes, undo formatting, and reapply changes to force push pr.
Blockers: None
Availability: Tues & Friday
ETA: 02/21/25
Pictures: N/A

@ryanfchase
Copy link
Member

Left my review here:

Since I have left another review, please provide another update for us:

Instructions
  1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
  2. Blockers: "Difficulties or errors encountered."
  3. Availability: "How much time will you have this week to work on this issue?"
  4. ETA: "When do you expect this issue to be completed?"
  5. Pictures (if necessary): "Add any pictures that will help illustrate what you are working on."
  1. Progress:
  2. Blockers:
  3. Availability:
  4. ETA:
  5. Pictures (if necessary):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

4 participants