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 NavigationBar column width modifier #50

Merged
merged 10 commits into from
May 22, 2024
Merged

Conversation

markstamer
Copy link
Collaborator

This Pull Request adds a modifier to set the columnWidth of the NavigationBar. The property was already present but not used. It only affects the horizontal placement of the navigation bar items.

Screenshot 2024-05-14 at 14 00 08

@twostraws
Copy link
Owner

Hello! Thank you for this. It would be helpful if you could perhaps explain what this does from a practical perspective – what is the problem right now, and what does this solution allow? I can't tell from the screenshot you've attached. I appreciate it's probably obvious to you, but it's less obvious for me 😅

@markstamer
Copy link
Collaborator Author

Sure! I would like to achieve a full width navigation bar with the title and navigation items being inset to the sites content width. The example image is a fixedTop navigation bar. I would prefer this layout for the default navigation bar but I couldn't find a way to make it expand the full width of the site without changing the sites content width, which I assume would force me to adjust the content width of each page.

For the example I used the modifier like this: .navigationBarColumnWidth(.count(context.site.pageWidth)). To me the naming column width is a little confusing but tried to stay consistent to the existing property.

It is a little off topic but I couldn't get a navigation bar with a bottom border to work either by trying things like .style("navbar border-bottom border-secondary"). I would be happy to create another PR if you could give me a hint on how to achieve this.

@twostraws
Copy link
Owner

Ah! I see now. Thank you! I tried it on the Ignite Samples site and it's definitely an improvement, although Bootstrap is still adding a little extra left and right padding that gives me an eye twitch 😅

As a BlockElement, NavigationBar already has a width() modifier that adjusts this value. It's ignored, as you say, but I'm wondering whether simply making it un-ignored – effectively half your PR – would be better than adding a special modifier to do the same thing. We could add a separate implementation of width() as an override from the protocol, but I think all it would change is "this affects the items in the bar, rather than the bar itself" as a documentation comment.

…igation-bar-modifier

Removes unnecessary navigation bar width modifier in favor of the default implimentation of width().
@markstamer
Copy link
Collaborator Author

Right! The default implementation for BlockElement was there all along. I just removed the modifier and added a custom implementation for width to document the behavior as you suggested.

@twostraws
Copy link
Owner

Perfect – thank you!

@twostraws twostraws merged commit 6089a95 into twostraws:main May 22, 2024
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.

2 participants