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(3000): Add sites sidebar for Toolbar launching #16389

Merged
merged 8 commits into from
Jul 10, 2023
Merged

feat(3000): Add sites sidebar for Toolbar launching #16389

merged 8 commits into from
Jul 10, 2023

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Jul 5, 2023

Changes

It wasn't possible to launch the Toolbar in PostHog 3000. With the new "Sites" feature, it is. You can still open your sites in a new tab, but also view them inside PostHog, with the toolbar on.

Screenshot 2023-07-05 at 14 09 49

Of course with iframes there are significant security considerations, and most importantly most web apps don't allow iframing by default. There's currently no education on resolving this in the app, but we can add it later on.

@Twixes Twixes mentioned this pull request Oct 19, 2023
41 tasks
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 2 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.

@Twixes Twixes requested a review from thmsobrmlr July 6, 2023 16:05
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.

I really like the site concept! Code looks solid, one suggestion to add an enum.

Functionality wise I'm still missing being able to add other URLs besides the suggested ones and to edit the existing ones. Most importantly I can't launch the toolbar for local development e.g. to disable posthog-3000.

@@ -210,12 +210,17 @@ export const sceneConfigurations: Partial<Record<Scene, SceneConfig>> = {
},
[Scene.Ingestion]: {
projectBased: true,
plain: true,
plain: 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an enum for the different states?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, enum might be overkill, but made this into variants. Take a look now

@Twixes
Copy link
Member Author

Twixes commented Jul 7, 2023

Good feedback about the lack of "New site" button being annoying, I have a branch in progress that adds that. Assuming that PR is the follow up to this one, do you think this is going to be good enough for now?

@Twixes Twixes requested a review from thmsobrmlr July 7, 2023 17:55
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.

Yep, good enough to go in. It's adding more functionality and I'm just complaining about missing things :)

@Twixes
Copy link
Member Author

Twixes commented Jul 10, 2023

BTW it is still possible to launch the site with toolbar in a new tab like in the classic navigation, it's in item menu. Maybe it was not clear that that includes the Toolbar, so renamed from "Open in new tab" to "Open with Toolbar in new tab". Alternatively maybe for important actions like that there could be an action button on items, which in this case would make "Open with Toolbar in new tab" prominent. (@corywatilo)

@Twixes Twixes enabled auto-merge (squash) July 10, 2023 10:40
@Twixes Twixes merged commit 72dcf73 into master Jul 10, 2023
@Twixes Twixes deleted the 3000-sites branch July 10, 2023 11:24
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