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

Add PrimaryNav feature #604

Merged
merged 7 commits into from
Feb 27, 2025
Merged

Add PrimaryNav feature #604

merged 7 commits into from
Feb 27, 2025

Conversation

reakaleek
Copy link
Member

@reakaleek reakaleek commented Feb 25, 2025

Add a feature flag PrimaryNav which enables the custom primary navigation and splits the pages navigation needed for production.
When it's disabled, it shows the full navigation (as before).

Configuration Enhancements:

  • Added a new Features dictionary to the ConfigurationFile class to support feature toggles.

Htmx Class Updates:

  • Updated the Htmx class to include methods that utilize the new Features dictionary for dynamic configuration of attributes.

Navigation and HTML Rendering Improvements:

  • Modified the HtmlWriter class to cache rendered navigation HTML and utilize the new configuration features for conditional rendering.
  • Updated various HTML rendering methods to use the new MarkdownParser instance method instead of static methods.

Markdown Parsing and Rendering:

  • Refactored the MarkdownParser class to use instance methods and accept a BuildContext for dynamic configuration.
  • Updated the HtmxLinkInlineRenderer class to use the new configuration features and dynamic attributes.

Miscellaneous:

  • Minor CSS adjustments to improve text wrapping and inline-block behavior.
  • Added support for trailing slashes in navigation item links in pages-nav.ts.
  • Ensured the current navigation item is scrolled into view and the window is scrolled to the top.

Results

PrimaryNav feature enabled

docs-content-nav.mp4

PrimaryNav feature disabled

docs-builder-nav.mp4

@reakaleek reakaleek marked this pull request as ready for review February 25, 2025 11:54
@reakaleek reakaleek marked this pull request as draft February 25, 2025 11:54
@reakaleek reakaleek changed the title Primary Nav PoC Add PrimaryNav feature Feb 27, 2025
@reakaleek reakaleek self-assigned this Feb 27, 2025
@reakaleek reakaleek force-pushed the feature/header-design-pt2 branch from edd2fc9 to 745486b Compare February 27, 2025 11:06
@reakaleek reakaleek marked this pull request as ready for review February 27, 2025 11:09
@reakaleek reakaleek requested review from Mpdreamz and a team February 27, 2025 11:09
@@ -133,23 +133,21 @@ jobs:
- name: Build documentation
if: github.repository != 'elastic/docs-builder' && (steps.deployment.outputs.result || (steps.check-files.outputs.any_modified == 'true' && github.event_name == 'merge_group'))
uses: elastic/docs-builder@main
id: docs-build
Copy link
Member

Choose a reason for hiding this comment

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

I think a merge went bad here? we want to keep these changes from main

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, something went wrong. Thank you for bringing this to my attention!

@@ -0,0 +1,16 @@
name: 'Documentation Generator'
Copy link
Member

Choose a reason for hiding this comment

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

likewise this should be deleted

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

docs-builder.sln Outdated
@@ -60,17 +60,6 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "validate-inbound-local", "v
actions\validate-inbound-local\action.yml = actions\validate-inbound-local\action.yml
EndProjectSection
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "update-link-index", "update-link-index", "{6554F917-73CE-4B3D-9101-F28EAA762C6B}"
Copy link
Member

Choose a reason for hiding this comment

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

keep the changes to this file

Copy link
Member Author

Choose a reason for hiding this comment

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

docs/_docset.yml Outdated
@@ -9,6 +9,10 @@ subs:
serverless-short: Serverless
ece: "Elastic Cloud Enterprise"
eck: "Elastic Cloud on Kubernetes"

#features:
# PrimaryNav: true
Copy link
Member

Choose a reason for hiding this comment

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

primary-nav? We default to kebab-case (dash-case) for yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

href="@Model.UrlPathPrefix/"
hx-get="@Model.UrlPathPrefix/"
hx-select-oob="@Htmx.GetHxSelectOob()"
href="@Model.Link("/")"
Copy link
Member

Choose a reason for hiding this comment

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

❤️ much needed

@reakaleek reakaleek requested a review from Mpdreamz February 27, 2025 12:40
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Two more nitpicks


string? navigationHtml;

if (DocumentationSet.Configuration.Features.ContainsKey("primary-nav"))
Copy link
Member

Choose a reason for hiding this comment

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

Lets have a static class with constant string properties so we can use Features.PrimaryNavigation.

Copy link
Member Author

@reakaleek reakaleek Feb 27, 2025

Choose a reason for hiding this comment

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

ee10395

Instead of a static constant I created a class.

which can be used like

if (features.IsPrimaryNavEnabled) {
 ...
}

WDYT?

Tree = group ?? DocumentationSet.Tree,
CurrentDocument = markdown,
IsRoot = topLevelGroupId == DocumentationSet.Tree.Id,
Configuration = configuration
Copy link
Member

Choose a reason for hiding this comment

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

Since we only use this in GetHxSelectOob to get the features, set the features on the model and not the whole Configuration?

Copy link
Member Author

Choose a reason for hiding this comment

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

@reakaleek reakaleek requested a review from Mpdreamz February 27, 2025 13:35
@reakaleek reakaleek merged commit 8edd22a into main Feb 27, 2025
5 checks passed
@reakaleek reakaleek deleted the feature/header-design-pt2 branch February 27, 2025 13:48
reakaleek added a commit that referenced this pull request Feb 27, 2025
reakaleek added a commit that referenced this pull request Feb 27, 2025
@reakaleek reakaleek restored the feature/header-design-pt2 branch February 27, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants