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 NavDivider for non-selectable menu entries #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AvdN
Copy link

@AvdN AvdN commented Aug 3, 2023

Adds a NavDivider dataclass that can be used in a nav_items list like a NavItem, only taking title (and icon). It will be rendered in the pull-down with a grey background, and is unclickable.

Styling can be changed with CSS: li[role=divider] { ...; }

As Jinja2 cannot test if a variable is an instance (beyond some built-ins) the divider attribute (default: True) is tested. Dataclasses do not have the option to make an attribute immutable, so this can be broken by assigning to that attribute.

Adds a `NavDivider` dataclass that can be used in a `nav_items` list
like a `NavItem`, only taking `title` (and `icon`). It will be rendered
in the pull-down with a grey background, and is unclickable.

Styling can be changed with CSS: `li[role=divider] { ...; }`

As Jinja2 cannot test if a variable is an instance (beyond some built-ins)
the `divider` attribute (default: `True`) is tested. Dataclasses do not
have the option to make an attribute immutable, so this can be broken
by assigning to that attribute.
@fscherf
Copy link
Member

fscherf commented Aug 3, 2023

At first glance, this looks good to me. The divider is hardcoded in CSS to be grey. Did you test it with light- and dark-theme?

@AvdN
Copy link
Author

AvdN commented Aug 3, 2023

No I haven't. I haven't got a switch in my test app anymore so I don't know how to go from one to another. Is there away to set that in the settings?

@fscherf
Copy link
Member

fscherf commented Aug 3, 2023

@AvdN
Copy link
Author

AvdN commented Aug 3, 2023

I now made it background-color: var(--form-element-disabled-background-color);, as in light mode the contrast was not enough. Dark mode also tested with this and is still ok (slightly less contrast than before).

@fscherf
Copy link
Member

fscherf commented Aug 3, 2023

@AvdN: The color looks good to me. Seems to be the appropriate color value. For me it looks like this:
2023-08-03_21-52

The divider is a bit thick for my taste. I would want to have something like bootstrap has:

2023-08-03_21-56

Also, the new class will need some documentation, or be at least mentioned in the example.

Besides that, good job!

@AvdN
Copy link
Author

AvdN commented Aug 3, 2023

Then maybe divider is not the right word, I use them to group elements (that might have the same name), without having to for sideways expanding pop-outs as some libraries have. The real thing has another divider with 2 more entries.
image

But I agree that a "real" divider, without context should be smaller.

@fscherf
Copy link
Member

fscherf commented Aug 3, 2023

Ah! You are using it more like another hierarchy level

@AvdN
Copy link
Author

AvdN commented Aug 3, 2023

Yeah, but if the title is empty a different rendering could be made, that looks more like the line from bootstrap. Do you have an online site where that is (instead of the image), so I can look at the HTML and the styling?

@fscherf
Copy link
Member

fscherf commented Aug 3, 2023

@AvdN
Copy link
Author

AvdN commented Aug 4, 2023

I looked at that and as I had tried already it uses a


element. But just inserting that doesn't work in picocss, the list element is still the same size as if it had text. Spend another two hours on this, and learned a lot more about inspect mode in Firefox, the :before pseudo elements (which AFAICT set the hight by providing content: "";), but never was able to get the
to display independent of being to high.

I can change the NavDivder name if you want (let me know what name), and/or extend the example in customization.md to use it.

I am not going to look into using an


any further, especially as the navbar, as-is, is unusable with the double click to go from main navbar item to another (i.e. I will look at bootstrap, or else try to get my current jQuery based stuff to work with Lona)

@fscherf
Copy link
Member

fscherf commented Aug 8, 2023

I did some experiments and I think we want three different things here:

  1. A third level of navigation. I think it would make more sense to have syntax like this:
return [                                                
    NavItem(                                            
        title='Components',                             
        nav_items=[                                     
            NavItem(                                    
                title='Forms',                          
                nav_items=[                             
                    NavItem(                            
                        title='Buttons',                
                        url='/components/forms/buttons',
                    ),                                  
                ],                                      
            ),                                          
        ],                                              
    ),                                                  
]                                                       

result in something like this: ("Forms" ist just a label, and can't be clicked)
2023-08-08_10-06

  1. A divider as a horizontal line. It is a related but different feature and could be useful in the future. The more I think about it I don't want a NavDivider class but a divider=True attribute in the NavItem class. Initially, I didn't want that because we will have to do some validity checks, because divider=True with an URL set makes no sense and should at least produce a warning. On the other hand, having a class for every new feature does not result in a nice developer experience (I am sorry I changed my mind on that)

  2. Navigation open on hover. To be honest I did not look into this, but shouldn't be too hard. I can do that. I am pretty deep in that part of the code anyways

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