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

fix: Some issues with onboarding for 3000 #18746

Merged
merged 21 commits into from
Nov 21, 2023
Merged

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Nov 20, 2023

Problem

Some remaining work to be done

Changes

  • Fixes the flex layout for the product cards
  • Fixes a bunch of button sizes and issues
  • Fixes the input selected to just use the border color
Screenshot 2023-11-20 at 17 20 31 Screenshot 2023-11-20 at 17 20 07

TODO

  • Fix the topbar not being shown for 3000
  • Confirm signup/login buttons look correct

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

How did you test this code?

Copy link
Contributor

github-actions bot commented Nov 20, 2023

Size Change: -27 B (0%)

Total Size: 2 MB

Filename Size Change
frontend/dist/toolbar.js 2 MB -27 B (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 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.

@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 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

benjackwhite and others added 2 commits November 20, 2023 15:13
# Conflicts:
#	frontend/__snapshots__/scenes-app-insights--funnel-left-to-right-breakdown-edit.png
#	frontend/__snapshots__/scenes-app-insights--funnel-top-to-bottom-breakdown.png
@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 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

7 snapshot changes in total. 0 added, 7 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 1)
  • webkit: 0 added, 0 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 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@benjackwhite benjackwhite marked this pull request as ready for review November 20, 2023 16:21
@benjackwhite benjackwhite requested a review from a team November 20, 2023 16:21
}

&.LemonInput--focused:not([aria-disabled='true']) {
border-color: var(--primary-3000);

.posthog-3000 & {
border-color: var(--border-bold);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this changes everywhere in 3000? Feels like a big change but understand the concerns about it looking like an error state. Just wondering if grey is the greatest active state

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 think it is miles better than orange (i.e. nearly red). So yeah I would vote for it as the default until we have a better option

@Twixes Twixes force-pushed the fix/onboarding-3000 branch from 13d110a to 862c722 Compare November 20, 2023 20:13
@raquelmsmith
Copy link
Member

raquelmsmith commented Nov 20, 2023

Poked at this a bit. I have code for the following changes to the top nav but will wait until I get the go/no-go from y'all before I push to this branch:

  1. Only show project switcher if the org has multiple projects
  2. Make the user button tertiary
  3. Make both buttons muted so the icon isn't red (not sure where we are on this from a 3000-colors perspective, but it felt really abrupt here)
  4. Added a border below the minimal nav. I'm kinda iffy on this because generally our top "nav" in 3000 (i think mostly breadcrumbs) doesn't have a border, but it looked kind of naked without, and since in this view this is the "primary" nav, I think the border is okay.
  5. Made the posthog logo/icon a bit larger

Only one project:
image

Multiple projects:
image

@benjackwhite benjackwhite enabled auto-merge (squash) November 21, 2023 08:28
@benjackwhite benjackwhite merged commit 0e15477 into master Nov 21, 2023
71 of 72 checks passed
@benjackwhite benjackwhite deleted the fix/onboarding-3000 branch November 21, 2023 08:40
@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 (wasn't pushed!)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@benjackwhite
Copy link
Contributor Author

@raquelmsmith I put in the changes as in your screenshots 👍

@raquelmsmith
Copy link
Member

awesome thanks so much!

@raquelmsmith
Copy link
Member

raquelmsmith commented Nov 21, 2023

Whoops, I think this broke onboarding - bridge pages are no longer scrollable. My fault really since I have no tests for onboarding flows.... I'm going to revert this since it's not super critical and then we can fix

raised by user in #18799

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.

4 participants