Skip to content

Refactor event history routing#984

Merged
Alex-Tideman merged 13 commits intomainfrom
refactor-event-routes
Dec 9, 2022
Merged

Refactor event history routing#984
Alex-Tideman merged 13 commits intomainfrom
refactor-event-routes

Conversation

@Alex-Tideman
Copy link
Copy Markdown
Collaborator

@Alex-Tideman Alex-Tideman commented Dec 8, 2022

What was changed

Remove slots for event history pages to prevent n + 1 growth of rerendering event history after navigating.

Essentially all this does is move everything into the [run]/__layout@root.svelte and I use $page.url.pathname.endsWith to determine whether to show feed/compact/json.

It does feel strange to need to keep empty feed/index.svelte / compact.svelte / json.svelte files just to let svelte know the route exists. But this way we don't need to change any url patterns.

Why?

Prevents grow of rerenderings

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
holocene ✅ Ready (Inspect) Visit Preview Dec 9, 2022 at 5:40PM (UTC)
ui ✅ Ready (Inspect) Visit Preview Dec 9, 2022 at 5:40PM (UTC)

@cypress
Copy link
Copy Markdown

cypress Bot commented Dec 8, 2022



Test summary

93 0 0 0


Run details

Project Temporal UI
Status Passed
Commit b3e9dfd805 ℹ️
Started Dec 9, 2022 5:41 PM
Ended Dec 9, 2022 5:45 PM
Duration 04:44 💡
OS Linux Ubuntu - 22.04
Browser Chrome 108

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Comment on lines +20 to +21
$: compactPage = $page.url.pathname.endsWith('compact');
$: jsonPage = $page.url.pathname.endsWith('json');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think we even need routes for these views? Maybe could just make the last selected view a sticky preference.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We don't, I was being overly cautious about removing them since there will be old links with /feed or /compact. What I did in the latest commit is remove them from any internal routes, but if a user does go to .../history/feed or .../history/compact or .../history/json, i redirect them to /history

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So I'm using the existing sticky view preference (eventViewType) to render the right view.

  const views = {
    feed: WorkflowHistoryFeed,
    compact: WorkflowHistoryCompact,
    json: WorkflowHistoryJson,
  };
  $: view = views[$eventViewType] ?? WorkflowHistoryFeed;

 <svelte:component this={view} />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Beautiful 🎨

Copy link
Copy Markdown
Contributor

@rossedfort rossedfort left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +26 to +28
<!-- <svelte:fragment slot="timeline">
<EventHistoryTimelineContainer />
</svelte:fragment> -->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be commented out?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah for the time being. I leave it commented out so we know where it goes once it's ready to flip on.


const workflow = $page.params.workflow;

const views = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🙌

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.

3 participants