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

feat: Pipeline UI App tabs with logs view #18527

Merged
merged 6 commits into from
Nov 17, 2023

Conversation

tiina303
Copy link
Contributor

@tiina303 tiina303 commented Nov 9, 2023

Problem

The single app view would allow editing the app config, viewing metrics and logs.
See storybook images in the files for what things look like.

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

5 snapshot changes in total. 0 added, 5 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@tiina303 tiina303 marked this pull request as ready for review November 9, 2023 20:01
@tiina303 tiina303 requested review from a team, mariusandra, Twixes and thmsobrmlr and removed request for mariusandra November 9, 2023 20:01
@tiina303 tiina303 force-pushed the pipeline-ui-transformations branch 5 times, most recently from 9704660 to 5984a99 Compare November 13, 2023 17:16
Base automatically changed from pipeline-ui-transformations to master November 14, 2023 13:58
@tiina303 tiina303 force-pushed the pipeline-ui-transf-single-page branch from 1745022 to 7a2e3f4 Compare November 15, 2023 15:56
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

Great work!

Left a couple of nit-picks inline.

frontend/src/scenes/pipeline/Transformations.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly part of this PR, but stumbled into it here. Imo it would be great to wrap the logs in something like the following to preserver whitespace and make it look more like code:

render: (message: string) => <code className="whitespace-pre">{message}</code>,
Screenshot 2023-11-15 at 18 47 37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure this out I tried whitespace-pre-wrap which helped as now the result wasn't just a single line if I printed out the whole event, but it was still too long, so break-all helped, but then it also broke in the middle of the words, which isn't great.

frontend/src/scenes/pipeline/pipelineAppLogic.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

Size Change: -2.21 kB (0%)

Total Size: 2.01 MB

Filename Size Change
frontend/dist/toolbar.js 2.01 MB -2.21 kB (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@tiina303 tiina303 merged commit b8c188c into master Nov 17, 2023
73 checks passed
@tiina303 tiina303 deleted the pipeline-ui-transf-single-page branch November 17, 2023 11:25
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