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: nav scope title + breadcrumbs #248

Merged
merged 33 commits into from
Feb 14, 2025
Merged

Conversation

davidlougheed
Copy link
Member

@davidlougheed davidlougheed commented Jan 23, 2025

No description provided.

@davidlougheed
Copy link
Member Author

requesting an initial pass - i know lint is failing right now

@gsfk
Copy link
Member

gsfk commented Feb 7, 2025

Works okay, no major issues on my end. I like the sticky part.

  • the page hierarchy is still shared with the side menu in a way that might not be obvious, but I still don't have fixes to suggest. I can't decide if this is better or not than putting the same text in the header.
  • when I first loaded the search page at full scope, I get this: "🔎 Search" ... and my first thought was that it was a search bar I was supposed to type search text in. I don't think this is a problem, but it's worth noting.

Copy link
Contributor

@SanjeevLakhwani SanjeevLakhwani left a comment

Choose a reason for hiding this comment

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

I like that it does address the major issues with navigation in public. Something about it ui wise feels a lil off but that is just me. I am satisfied with it that I would be happy with it being a part of public. (Haven't yet looked at the code).
Couldn't find any behavioral bugs with it for now. Also, it would be great to have cta buttons for overview and search in provenance of projects.

@davidlougheed
Copy link
Member Author

what do you mean by "Also, it would be great to have cta buttons for overview and search in provenance of projects."? sorry, a little confused there

and let me know if you figure out what bugs you about the UI stuff

@davidlougheed davidlougheed marked this pull request as ready for review February 10, 2025 21:34
@davidlougheed davidlougheed changed the title feat: experiment with nav scope breadcrumbs feat: nav scope title + breadcrumbs Feb 11, 2025
Copy link
Contributor

@SanjeevLakhwani SanjeevLakhwani left a comment

Choose a reason for hiding this comment

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

First pass.
Somethings felt like they could be split up into different PRs. Don't hesitate to make different PRs for separation of concerns, as it doe help in better and faster reviews.(And also increases the rate at which new code is adopted).

Copy link
Contributor

@SanjeevLakhwani SanjeevLakhwani left a comment

Choose a reason for hiding this comment

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

lgtm!

@davidlougheed davidlougheed merged commit 07b902b into main Feb 14, 2025
2 checks passed
@davidlougheed davidlougheed deleted the feat/experiment-breadcrumbs branch February 14, 2025 14:59
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