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

Fix some glaring performance issues in history view as we prepare to release it #1545

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Fix some glaring performance issues in history view as we prepare to release it #1545

wants to merge 12 commits into from

Conversation

travjenkins
Copy link
Member

@travjenkins travjenkins commented Apr 1, 2025 •
edited
Loading

Issues

Contributes: #1369

Changes

1369

  • No longer fetch ALL the specs on the load of the page
  • Break up components a bit
  • Add query params into URL to potentially allow sharing of compare links easier
    • Storing them in two params so we can eventually allow user to select any two to compare
  • Tweaking styles to make it more clear the selected pub is on the right side.
  • Make sure that onload the selected publication is visible with scrollIntoView

Tests

Manually tested

  • scenarios you manually tested

Automated tests

  • unit testing covered

Playwright tests ran locally

  • Admin
  • Captures
  • Collections
  • HomePage
  • Login
  • Materialization

Screenshots

Onload it will scroll smoothly down to the item selected
image

travjenkins added 10 commits April 1, 2025 13:08
@travjenkins
Update history view to be more performant
ec2ffbd 
 - Stop fetching all of history on load of page
 - Fetch each spec individually when a users selects publication
 - Break out the diff viewer into a stand alone component

Typing
 - Some types were off for the history page so updated those
@travjenkins
Updating to latest @monaco-editor/react
b37ebce
@travjenkins
I think showing progress while keeping editor is way less shocking UX
420cf50
@travjenkins
Updating how we handle if there is only one pubId
53f9b73 
Minor tweaks to the UX for loading and comparing
Trying to make it more clear the selected item is on the right of the diff
@travjenkins
Updating data fetching
71ca1d3 
 - We can just use the `published_at` from the main list
 - Make the function safer so it only calls when there is
    at least _one_ pub id

Editor UX
 - Keep the viewState while changing the model so the scroll stays
     while changing publications
@travjenkins
Noting down an issue with the underlying lib
d3bf670
@travjenkins
Getting the back button working
c0addee
@travjenkins
Some renaming and refactoring
3b5fed8 
 - moved stuff into a shared hook to keep things consistent
 - rename the query params to be specific to this page
 -
@travjenkins
Breaking the hook into two
18d7f3c
@travjenkins
removing cursor when the item is selected
2fec3b9
@travjenkins travjenkins mentioned this pull request Apr 2, 2025
Open
3 tasks
@travjenkins travjenkins marked this pull request as ready for review April 2, 2025 17:01
@travjenkins travjenkins requested a review from a team as a code owner April 2, 2025 17:01
kiahna-tucker
kiahna-tucker approved these changes Apr 3, 2025
View reviewed changes
Copy link
Member

@kiahna-tucker kiahna-tucker left a comment

Choose a reason for hiding this comment

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

Approved with moderate testing.

@travjenkins travjenkins added the change:planned This is a planned change label Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiahna-tucker kiahna-tucker kiahna-tucker approved these changes

Assignees
No one assigned
Labels
change:planned This is a planned change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@travjenkins @kiahna-tucker