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): Show the 3000 index-less navbar with labels #18263

Merged
merged 14 commits into from
Nov 2, 2023

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Oct 30, 2023

Changes

We want to initially release the 3000 look with labels in the navbar, not too dissimilar to what the app looks like now, so that the change isn't disorienting. This PRs add the labels, along with a few navbar items that were missing.

Note: when the posthog-3000-nav flag is active (i.e. sidebar-based navigation), the buttons are still icons.

@Twixes Twixes requested a review from a team October 30, 2023 10:30
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Love it!

We discussed perhaps trying out the resizer thing in a follow up once this and #18204 are merged 👍

{navbarItems.map((section, index) => (
<ul key={index}>
{section.map((item) =>
item.featureFlag && !featureFlags[item.featureFlag] ? null : (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice featureFlag helper!

@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.

@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 enabled auto-merge (squash) October 31, 2023 19:08
@corywatilo corywatilo disabled auto-merge November 1, 2023 17:05
@corywatilo
Copy link
Contributor

corywatilo commented Nov 1, 2023

A few suggestions:

  1. Replace "Project homepage" with the actual project name
  2. Persons and groups -> People
  3. Just move Cohorts into People now
  4. Data management -> Data
  5. Just move Annotations into Data now
  6. Support beta/alpha labels?
  7. The second section is intended for products, so I'd move Toolbar out to the lower section
  8. Alphabetize products
  9. Project settings -> Settings

If we make these changes now, we'll be pretty close to what we'll release in full 3000, so we can just have a tooltip/popup saying "We moved some things around! Data is up top, products are in the middle. Cohorts now live in Data, and Cohorts moved to People"

@Twixes
Copy link
Member Author

Twixes commented Nov 2, 2023

Thanks for considering this! @corywatilo
Tried addressing all your points. With three I ran into some blocking trickiness (marked with orange):

  1. ➡️ I think that makes sense, though it should happen alongside the project name being pulled out of the breadcrumbs bar, so will do that in a separate PR.

  2. ❇️ Agreed with "People", but a bit wary of removing "groups" from that label, because 1. makes it harder to learn about group analytics 2. in our model groups are not made up of people – they're made up purely out of events. Named "People and groups" for now.

  3. ➡️ Cohorts and annotations are being moved in feat: Sidebar cohorts and annotations movement #18200.

  4. 🆚 I don't quite see renaming "Data management" to "Data" improving clarity. I mean, "Data" is more concrete, but it's also concretely not what's on that page. The data can be found in "People and groups", and "Event explorer". "Data management" is basically "Data taxonomy" though – perhaps that'd be clearer?

  5. ➡️ Same as 3.

  6. ✅ Good catch, added alpha and beta labels. They feel too prominent in dark mode, but that's a color scheme issue…
    I came up with an outlined style for that case, but it's not legible in light mode, and not sure if it'll work outside of the navbar in dark. Hence leaving out of this PR.
    Screenshot 2023-11-02 at 20 03 10

  7. ✅ Put Toolbar in the top segment.

  8. 🆚 One problem I see with alphabetic sorting is that related products would be far apart, e.g. "Product analytics" would be separated from "Web analytics" by "Session replay" + "Surveys". I don't feel like this is outweighed by the alphabetic aspect.

  9. ✴️ The "Project" part in "Project settings" is currently pretty key, because a user might easily think that's where you can find all the settings or at least navigate wherever you need. That's not the case, there are separate entry points (i.e. buttons) for organization and account settings. It would be nice to be able to treat this button as just "Settings", but currently we can't – we'll need to solve this as part of designing settings at some point (as the current experience is the result of pure organic growth).

Current state:

@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 merged commit 243cf56 into master Nov 2, 2023
73 checks passed
@Twixes Twixes deleted the 3000-full-sidebar branch November 2, 2023 21:38
@corywatilo
Copy link
Contributor

Settings

Just FYI I do have some thoughts on the settings UI here.

@Twixes
Copy link
Member Author

Twixes commented Nov 2, 2023

Deployed this, but obviously happy to follow up on all the remaining (orange) points! Settings very much included.

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