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

Breadcrumbs #6770

Merged
merged 25 commits into from
Nov 10, 2021
Merged

Breadcrumbs #6770

merged 25 commits into from
Nov 10, 2021

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Nov 1, 2021

Changes

🍞crumbs. WIP.

Base automatically changed from help-popup-refresh to fresh-layout-sidebar November 1, 2021 19:02
Base automatically changed from fresh-layout-sidebar to master November 3, 2021 10:43
@Twixes
Copy link
Member Author

Twixes commented Nov 8, 2021

So, help yourself to some crumbs. Try this out locally and let me know what you think about the experience!
Here are some previews:

  • Organization-based:
    Zrzut ekranu 2021-11-8 o 19 06 27
  • Project-based, level 0:
    Zrzut ekranu 2021-11-8 o 19 02 47
  • Project-based, level 1:
    Zrzut ekranu 2021-11-8 o 18 45 29
  • Instance-level (system status or licenses):
    Zrzut ekranu 2021-11-8 o 18 37 28
  • Personal (i.e. account settings):
    Zrzut ekranu 2021-11-8 o 18 36 24

@Twixes Twixes marked this pull request as ready for review November 8, 2021 22:49
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

Looking great, did an initial pass at this. Some comments inline. In addition, some UX issues:

  1. I strongly recommend adding the current view as part of the breadcrumbs (something like this), otherwise it's hard to know exactly where you are.

  1. It was confusing that some breadcrumbs are underlined and others not (see comment 3 too). In addition, "Insights" label is particularly confusing in this context, I wonder if for breadcrumbs we should call this "Insight list". Thoughts @clarkus ?

  1. Intuitively I would expect to be able to switch the project/org from here, if I'm seeing the name and context here.

  1. Breadcrumbs disappear on actions. Further, I would probably expect to see Actions as a separate thing, i.e. Hogflix > Demo app > Events > Actions.

frontend/src/layout/lemonade/SideBar/index.scss Outdated Show resolved Hide resolved
@clarkus
Copy link
Contributor

clarkus commented Nov 9, 2021

  1. I strongly recommend adding the current view as part of the breadcrumbs (something like this), otherwise it's hard to know exactly where you are.

The header is part of this pattern. It's meant to represent the current item. I do not think we should repeat that in the breadcrumbs.

  1. It was confusing that some breadcrumbs are underlined and others not (see comment 3 too). In addition, "Insights" label is particularly confusing in this context, I wonder if for breadcrumbs we should call this "Insight list". Thoughts @clarkus ?

I think these should match the names in the navigation. I'd rather go with "Insights" and help users learn what that implies. Also once you click it, you can easily confirm it's a list of insights.

  1. Intuitively I would expect to be able to switch the project/org from here, if I'm seeing the name and context here.

Not part of this by design for now. It might be good to add this in the future if it turns out to be a problem for users (I already have designs for it), just don't want to overload this component for now. The main job of the breadcrumbs is to give you a better sense of current place, not so much to offer parity with the collapsed navigation.

  1. Breadcrumbs disappear on actions. Further, I would probably expect to see Actions as a separate thing, i.e. Hogflix > Demo app > Events > Actions.

I have an updated page design for this feature at https://www.figma.com/file/9yWtngNb1AIuf6KmXaEPJA/App-doodles?node-id=1121%3A143574. I think we'd just show the org and project in the breadcrumbs here (much like we do for insights). The header would set the context of Events & Actions, and the tabs would then take over setting the specific feature section.

One thing we should change is the placement of that demo banner. That's mean to be page level, so ideally it'd be shown above the page header or below it. Header here would apply to anything from the breadcrumbs down to the horizontal rule used to delimit the page content from the header.

@mariusandra
Copy link
Collaborator

mariusandra commented Nov 9, 2021

  1. Some issues with responsiveness
    image

  2. Personally, I'm in the same boat as @paolodamico and it feels really weird that the breadcrumbs don't include the current URL. I believe it impedes usability... and this is also the approach recommended by my go to source for UX guidelines (NN Group). Point nr 4: Include the current page as the last item in the breadcrumb trail.

I would even go so far to say that I would prefer if we remove the page title, and put it in the breadcrumbs... versus not having the page title in the breadcrumbs. We're duplicating information here anyway, so what's one extra link. Plus it really makes navigation between pages so much more intuitive, as you have all the information in just one condensed row.

  1. Going Dash -> Insight makes the crumbs change in a weird way
    2021-11-09 10 31 25

(made worse by the fact that the current page is not in the breadcrumbs)

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Officially requesting changes as well, to prevent the github bot from reminding me of this.

@Twixes
Copy link
Member Author

Twixes commented Nov 9, 2021

Thank yall for the feedback!
I've acted upon it in the following way:

  1. Added current location as the final breadcrumb. It shows the current item's name/key, except for the insight view, which doesn't have a solid way of getting the currently edited item from outside the page (fromItem crap which is due for being significantly refactored anyway for cleaner URLs).
  2. Removed underlines from links. Though now it's a bit less clear which breadcrumb items are links.
  3. Did NOT add org/project switching. It would require expanding the scope significantly, while also introducing complexities like where to redirect when switching on a specific item's page (i.e. feature flag foo may exist in a project, but likely doesn't in another one).
  4. Made sure breadcrumbs work on all Events & actions tabs.
  5. Improved responsiveness by making the breadcrumbs x-scrollable instead of having them shrink awkwardly.

As for the Dashboards → Insights transition @mariusandra I think it's reflecting what's happening pretty well. Insights can be on dashboards (that's what the "On dashboard" button is), but they don't belong to them. Especially since we plan to add support for a many-to-many relationship between these two models.

@Twixes
Copy link
Member Author

Twixes commented Nov 9, 2021

Examples:
Zrzut ekranu 2021-11-9 o 16 08 56
Zrzut ekranu 2021-11-9 o 16 25 24

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

This all looks good to me. I'd still find a way to get the true insight name inside the breadcrumbs when the URL refactor is done... but that's not today.

Other suggested changes:

  1. Change "Session recordings" to "Recordings"
  2. Change "Saved insights" to "Insights"

@clarkus
Copy link
Contributor

clarkus commented Nov 9, 2021

Personally, I'm in the same boat as @paolodamico and it feels really weird that the breadcrumbs don't include the current URL. I believe it impedes usability... and this is also the approach recommended by my go to source for UX guidelines (NN Group). Point nr 4: Include the current page as the last item in the breadcrumb trail.

@mariusandra This feels more targeted to designs that separate page headers from breadcrumbs. We're not doing that. They're always shown together as a single header component. If they were shown separately, I'd be more inclined to repeat this, but based on our app hierarchy and the placement of this component I don't think we need to repeat the current page. Can you elaborate on the specific usability problems you're concerned about?

Added current location as the final breadcrumb. It shows the current item's name/key, except for the insight view, which doesn't have a solid way of getting the currently edited item from outside the page (fromItem crap which is due for being significantly refactored anyway for cleaner URLs).

@Twixes I don't think we should do this. The header is meant to be an extension of the breadcrumbs. The header is a bold, prominent representation of the current location. I don't see what problem we're solving by repeating it in the breadcrumbs.

Removed underlines from links. Though now it's a bit less clear which breadcrumb items are links.

Shouldn't they all be links? Is this specific to showing the current item in the list of crumbs? If so, see my comment above. Not sure we should do this.

Copy link
Contributor

@clarkus clarkus left a comment

Choose a reason for hiding this comment

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

Just making my comments about the current breadcrumb item an official request change for github.

@mariusandra
Copy link
Collaborator

@clarkus It's hard to put a finger on it. Basically, for me the breadcrumbs are more or less useless without the final page in them, since I don't mentally register them as anything hierarchical without having an mental anchor to the current page in the list of links. I have no way of understanding that the current links are "everything up to the current page". I see the big title, and I know that's the title, but the font and style are so different, that the title and the crumbs could as well belong to a different and unconnected part of the page.

Without the current page in the breadcrumbs list, I'm personally 99% sure I'll use the sidebar or the back/forward buttons when I need to navigate, and the crumbs will just waste valuable vertical space.

That's my personal opinion, yet I'm quite sure the quote from the NNGroup site is for any kind of breadcrumbs, not for when they're followed by a big title.

@mariusandra
Copy link
Collaborator

If we're worried about duplicating content... then, well, we're duplicating everything else in the breadcrumbs anyway, so why not make it complete?

@clarkus
Copy link
Contributor

clarkus commented Nov 9, 2021

Without the current page in the breadcrumbs list, I'm personally 99% sure I'll use the sidebar or the back/forward buttons when I need to navigate, and the crumbs will just waste valuable vertical space.

The main purpose of the breadcrumbs is to give the user a sense of place - it's absolutely redundant with the left navigation, but that navigation is shown conditionally. It's meant to confirm that you're looking at the right thing when you have that nav collapsed. Yes it helps you get back quickly to the parent items, but you will need to use the navigation to get anywhere not in that hierarchy chain.

If we're worried about duplicating content... then, well, we're duplicating everything else in the breadcrumbs anyway, so why not make it complete?

Because this is repetition within the same component. You're literally saying Organization > Project > Container > CurrentThing > CurrentThing. My point is that the header is necessary to balance out placement of other page level actions, so taking that CurrentThing and giving it more prominence accomplishes the same task. The header is very much intended to be that current item.

I drew the proposed breadcrumb solution to illustrate why I think it's a dead end. What does repeating Insight #1234 accomplish over the header here? Is the heading using the same text value not achieving the same goal?

Screen Shot 2021-11-09 at 9 25 36 AM

@Twixes
Copy link
Member Author

Twixes commented Nov 9, 2021

I do agree with Chris's point about duplication (+ there's technical complexity in using internal data like insight name + inconsistencies in formatting can easily creep in e.g. "Events Stats" vs "Event stats"). Though I can see why some users (like Paolo and Marius) may have a better experience with that crumb. It seems that both sides feel pretty strongly about this so maybe we should just ask a couple more PostHog users to try this out and report feelings?

Shouldn't they all be links?

@clarkus This is only loosely related to the final breadcrumb issue. The reason why in practice most of the breadcrumbs are not links is because the top-level ones (organization + project OR instance OR user) aren't. They are logical pointers, but not concrete ones. I am a bit afraid that it may hinder usage of those breadcrumbs that are links (pointing to concrete pages) if users are unsure what is clickable, but I have no great solution for that at the moment.

@clarkus
Copy link
Contributor

clarkus commented Nov 9, 2021

It seems that both sides feel pretty strongly about this so maybe we should just ask a couple more PostHog users to try this out and report feelings?

I am very happy and willing to change my stance if we have information around a problem this addresses. I just can't see the problem we're addressing. User's might have opinions, but I feel like it should be based on a clear problem first.

@clarkus This is only loosely related to the final breadcrumb issue. The reason why in practice most of the breadcrumbs are not links is because the top-level ones (organization + project OR instance OR user) aren't. They are logical pointers, but not concrete ones. I am a bit afraid that it may hinder usage of those breadcrumbs that are links (pointing to concrete pages) if users are unsure what is clickable, but I have no great solution for that at the moment.

I think it'd be fine to make those look look like unlinked text nodes in that case. We might eventually have more things built out around collaboration that gives us meaningful places to link to. Until then it's ok if they're not actionable. The core job is to show you where you are in the current context. If anything, an item looking like it isn't a link is a better UX because you know you can't click on the thing.

@mariusandra
Copy link
Collaborator

Well, I still believe that without seeing a bold activated item inside the breadcrumbs, like here:

image

... the breadcrumbs lose all meaning and just become a random selection of loosely disjoined links that are mostly not even links.

The header below the crumbs has a wildly different style, and you can't mentally link it into the breadcrumb navigation structure without stopping to think... which equates with confusing UX.

That's also what the UX guidelines on the internet say, so ...

I'm fine to go against common sense and remove them if this moves the project along, though it might be better to have a flag that enables/disables the last item, and ask for feedback from the rest of the company.

@clarkus I think the screenshot above could use a good bit of padding between the breadcrumbs and the header, then it won't look so cramped. More aggressively, I would even consider removing the header, as with it being duplicated in the breadcrumbs, we could save some vertical space.

Here are a few cases where having the breadcrumb looks perfectly fine to me (changing the last item style compared to what's in this PR):

image

image

image

image

but of course if the header is so close to the crumbs it looks weird:

image

@posthog-contributions-bot
Copy link
Contributor

This issue has 2118 words at 12 comments. Issues this long are hard to read or contribute to, and tend to take very long to reach a conclusion. Instead, why not:

  1. Write some code and submit a pull request! Code wins arguments
  2. Have a sync meeting to reach a conclusion
  3. Create a Request for Comments and submit a PR with it to the meta repo or product internal repo

Is this issue intended to be sprawling? Consider adding label epic or sprint to indicate this.

@Twixes
Copy link
Member Author

Twixes commented Nov 10, 2021

I've conducted qualitative research by means of messaging random PostHoggers who happened to be online on Slack. The subjects were presented with two demo GIFs showcasing a specific breadcrumbful user flow: one without that final breadcrumb, the other with it. The result is: 2 out of 2 subjects were strongly in favor of the "You are here" breadcrumb. Let's keep it then.

@Twixes Twixes merged commit b01c2cd into master Nov 10, 2021
@Twixes Twixes deleted the breadcrumbs branch November 10, 2021 12:18
@mariusandra
Copy link
Collaborator

The one change I'd make is to make the last part look like on the mocked screenshots I posted, and like figma: just black bold text instead of blue italic.

@paolodamico
Copy link
Contributor

@mariusandra, you've said it all (it's as if we read the same book), putting out a PR to propose those changes and address a few other bugs I noticed, #7025. Great approach too @Twixes!

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