Skip to content

Add ability to include navigation bar items outside of the collapsible menu.#786

Closed
JPToroDev wants to merge 10 commits intotwostraws:mainfrom
JPToroDev:grid
Closed

Add ability to include navigation bar items outside of the collapsible menu.#786
JPToroDev wants to merge 10 commits intotwostraws:mainfrom
JPToroDev:grid

Conversation

@JPToroDev
Copy link
Copy Markdown
Collaborator

This PR adds the ability to include trailing items to a navigation bar that will not be included in the hamburger menu on small screen sizes—perfect for controls like Form and Button.

@JPToroDev JPToroDev closed this Apr 10, 2025
JP Toro added 3 commits April 12, 2025 09:45
#Conflicts:
#	Sources/Ignite/Elements/NavigationBar.swift
#	Sources/Ignite/Framework/ElementTypes/HTML.swift
@JPToroDev JPToroDev reopened this Apr 12, 2025
@twostraws
Copy link
Copy Markdown
Owner

Thank you for your patience! I'm back from vacation now, and getting through emails/PRs across various projects.

Rather than a second trailing closure, I wonder whether a modifier on items might work better? I'm thinking something like this:

NavigationBar(logo: "My Awesome Site") {
    Link("Tables", target: TableExamples())
        .navigationBarVisibility(.always)
}

The default would be .automatic. This approach might make it more natural to add/remove things from the visible set.

I'm not saying one option is better than the other; just looking for alternatives!

@JPToroDev
Copy link
Copy Markdown
Collaborator Author

I hope you had a great vacation, Paul—I know it was well deserved!

We’re on the same wavelength here. When I first closed the PR it was to experiment with a modifier-based approach.

There was something in the implementation at the time that made it clunky, but that obstacle’s since been removed.

So perhaps something like:

navigationItemPlacement()

with options like .callToAction and .primary (.automatic)?

I’ve conformed hidden() to NavigationItem in some upcoming enhancements to completely hide nav items at specific breakpoints, and something like the above modifier might create better distinction.

What do you think?

@JPToroDev
Copy link
Copy Markdown
Collaborator Author

Actually, with the introduction of Spacer as a NavigationItem, the modifier approach might still be a little clunky.

@twostraws
Copy link
Copy Markdown
Owner

Actually, with the introduction of Spacer as a NavigationItem, the modifier approach might still be a little clunky.

My thinking is that at rendering time, we filter the items array twice, once to look for always placements, and one to look for automatic placements, then effectively recreate the second array of items at that point. How would Spacer affect that?

with options like .callToAction and .primary (.automatic)?

I'm not sure either of those really tell me what the behavior is. Are there two behaviors here, or three?

  • Two behaviors: A thing is always visible, or only visible in expanded navbars
  • Three behaviors: A thing is always visible, only visible in expanded navbars, or never visible in expanded navbars. (This seems dubious?)

"Call to action" isn't giving me "this will always be visible" vibes, and "primary" feels like "most important" rather than "this will be hidden when collapsed."

@JPToroDev
Copy link
Copy Markdown
Collaborator Author

My thinking is that at rendering time, we filter the items array twice...

For sure—the technical side is straightforward either way.

I'm not sure either of those really tell me what the behavior is.

Agreed, mirroring SwiftUI isn't best here.

If we went with a modifier-based approach, perhaps this would be better?

navigationItemBehavior()
.permanent
.collapsible

.automatic would be synonymous with .permanent.

Are there two behaviors here, or three?

Just two behaviors:

  1. Permanent items that are rightmost and always visible. At small screen sizes, they are left of the toggle menu for a nicer UI.
  2. Collapsible items, which precede permanent items and collapse into the toggle menu at small screen sizes.

These two images showcase these behaviors:

CleanShot 2025-04-22 at 11 00 26@2x
CleanShot 2025-04-22 at 11 00 19@2x

The nav bar above is made from code like this:

NavigationBar {
    Link(“Home”, target:/)
} actions: {
    Link(“Download”, target:/)
        .linkStyle(.button)
    Spacer()
}

A modifier-based approach would look like this:

NavigationBar {
    Link(“Home”, target:/)
    
    Spacer()
        .navigationItemBehavior(.permanent)

    Link(“Download”, target:/)
        .linkStyle(.button)
        .navigationItemBehavior(.permanent)
}

But users could also write that code other ways, like the following, which might lead to things getting muddied:

NavigationBar {
    Spacer()
        .navigationItemBehavior(.permanent)

    Link(“Home”, target:/)
    
    Link(“Download”, target:/)
        .linkStyle(.button)
        .navigationItemBehavior(.permanent)
}

@twostraws
Copy link
Copy Markdown
Owner

This just isn't working for me. When I see "navigation item behavior permanent," my brain immediately tries to figure out what permanent behavior means – what behavior is being made permanent? To me this is a question of visibility. Is there a reason why .navigationBarVisibility(.always) doesn't work for you?

@JPToroDev
Copy link
Copy Markdown
Collaborator Author

JPToroDev commented May 1, 2025

Besides the modifier approach's obfuscating readability, I think this has the potential to be confusing:

Link(“Home”, target:/)
    .navigationBarVisibility(.always)
    .hidden(small: true) // sets ‘display: none’ at different breakpoints

But if you don’t think so, I’m fine with it.

@twostraws
Copy link
Copy Markdown
Owner

Well, it's that or something like navigationItemDisplayPriority(.high), or maybe even navigationItemPinned()? I've seen SwiftUI use "preferred" a few times when something is requested but not guaranteed.

However, if someone says "make it always visible, but hidden when in small mode," I'm not sure that's strictly a problem?

@JPToroDev
Copy link
Copy Markdown
Collaborator Author

I think either .navigationBarVisibility(.always) or navigationItemPinned() is best. I'll defer to you.

With the modifier approach, I think it'd be nice to add a NavigationItemGroup type, too.

Does that sound like a good path forward?

@twostraws
Copy link
Copy Markdown
Owner

Okay, let's go with .navigationBarVisibility(.always) and see what kind of feedback we get. Thank you!

@JPToroDev JPToroDev closed this May 2, 2025
@JPToroDev JPToroDev deleted the grid branch May 2, 2025 16:24
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