Skip to content

Add dropdown navigation per #85 - #106

Closed
ethanpil wants to merge 2 commits into
picocss:devfrom
ethanpil:patch-1
Closed

Add dropdown navigation per #85#106
ethanpil wants to merge 2 commits into
picocss:devfrom
ethanpil:patch-1

Conversation

@ethanpil

@ethanpil ethanpil commented Jan 2, 2022

Copy link
Copy Markdown

Per the discussion in #85 here is a proposed solution. I tested it with the default example file by adding this code right after the first <main class="container"> tag:

 <!-- Nav with Dropdown -->
<section id="nav-with-dropdown">
        <h2>Nav with Dropdown</h2>
        <nav>
          <ul>
            <li><a href="#">Computers</a></li>
            <li><a href="#">Tablets</a></li>
            <li>
              <a href="#">Phones</a>
              <ul>
                <li><a href="#">Apple</a></li>
                <li><a href="#">Samsung</a></li>
                <li><a href="#">Google</a></li>
                <li><a href="#">Sony</a></li>
              </ul>
            </li>
            <li><a href="#">Accessories</a></li>
          </ul>
        </nav>
      </section>

@lucaslarroche

Copy link
Copy Markdown
Member

Hi @ethanpil,
I will review and merge it.
Probably this weekend. At worst the next.

@lucaslarroche

Copy link
Copy Markdown
Member

Hi @ethanpil,

I like this addition, but there are a few problems:

  • The Dropdown can't be transparent as we can have other elements below. Actually, to manage properly the overlay, we probably need a box-shadow too
  • The submenu should be left-aligned to the parent link (and right-aligned in ltr mode)
  • This feature should work on vertical nav (see <aside> in the doc)
  • Paddings should be a multiple of the CSS var var(--spacing)
  • The font-size should probably stay at 1rem
  • We need to upgrade the documentation

I appreciate your PR, but I'd rather ship a more complete feature.
There is no emergency.

@lucaslarroche lucaslarroche added the work in progress This PR cannot be merged as is label Jan 9, 2022
@ethanpil

ethanpil commented Jan 9, 2022

Copy link
Copy Markdown
Author

Good points @lucaslarroche
Give me a few days and I will work on it.

In terms of your comment "The submenu should be left-aligned to the parent link (and right-aligned in ltr mode)"
I understand the left/right but not "to the parent link." Just doing a quick check, the documentation/example pages for both Bootstrap and Bulma don't align exactly to the link... the submenus have padding...

@jubishop

jubishop commented Jan 9, 2022

Copy link
Copy Markdown
Contributor

Thanks @ethanpil! I will greatly appreciate this addition

@lucaslarroche

lucaslarroche commented Jan 10, 2022

Copy link
Copy Markdown
Member

@ethanpil
Yes, you are right. With a border and/or box-shadow, we should have some paddings.
Screen-Shot-2022-01-10-at-10 13 06

Probably calc(var(--spacing) * 0.5)

Added box-shadow, rtl alignment, works with vertical nav/aside, fix paddings and font-size, per @lucaslarroche suggestions
@ethanpil

Copy link
Copy Markdown
Author

@lucaslarroche I just pushed in the changes you requested above. I built the css and replicated my results in this JSFiddle.

@lucaslarroche

Copy link
Copy Markdown
Member

Hi @ethanpil,

Thank you.
I don't have time to do a proper review this week. It will probably be next week.
I don't want to rush too much on a non-mandatory feature.
I preferred to concentrate on the fixes as it impacts all Pico users.

@lucaslarroche lucaslarroche mentioned this pull request Jan 23, 2022
@lucaslarroche
lucaslarroche changed the base branch from dev-to-delete to dev January 23, 2022 06:32
@lucaslarroche lucaslarroche mentioned this pull request Feb 27, 2022
@lucaslarroche

Copy link
Copy Markdown
Member

Hi @ethanpil,

After more thinking, researching, and iterating on @KiranMantha's PR, I changed the Dropdown integration.

Unfortunately, the next minor release (1.5.0) will not include your PR.

I think it's better for Pico's evolution as:

  • Dropdowns work inside and outside the <nav> component
  • Dropdowns can be styled as <button>
  • Using <details> allows us to trigger Dropdowns onclick even without JavaScript (and not just on hover)

Preview is here: https://picocss.github.io/pico/docs/dropdowns.html

Feel free to review and comment.
Thank you for your contribution.

@jubishop

Copy link
Copy Markdown
Contributor

Thanks Lucas! It'll be worth the wait :)

@ethanpil

Copy link
Copy Markdown
Author

@lucaslarroche I agree that @KiranMantha has a more flexible and robust solution. Glad that its there and happy to see the functionality in production ASAP!

@ethanpil ethanpil closed this Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

work in progress This PR cannot be merged as is

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants