Skip to content
This repository was archived by the owner on Feb 15, 2022. It is now read-only.

Basic mobile dropdown menu #209

Merged
3 commits merged into from
Mar 23, 2021
Merged

Basic mobile dropdown menu #209

3 commits merged into from
Mar 23, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 23, 2021

Adresses parts of #172 #125

Addresses:

Screenshot from 2021-03-23 17-53-03

Screenshot from 2021-03-23 17-54-49

@vercel
Copy link

vercel bot commented Mar 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/solvuu/ocamlorg2/7DTrhCJmPadTqAKaBD3g9izFRsvy
✅ Preview: https://ocamlorg2-git-kw1-mobile-nav-flyout-solvuu.vercel.app

</button>
<div className={"absolute z-10 left-1/2 transform -translate-x-1/2 mt-3 px-2 w-screen max-w-xs sm:px-0 " ++
switch activeMenu {

Copy link
Author

@ghost ghost Mar 23, 2021

Choose a reason for hiding this comment

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

Start of new code

let showMobileMenu = _evt => {setMobileDropdownVisible(_ => true)}

<div className="relative">
<div className="max-w-7xl mx-auto px-4 sm:px-6 ">
Copy link
Author

@ghost ghost Mar 23, 2021

Choose a reason for hiding this comment

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

End of new code

Copy link
Author

Choose a reason for hiding this comment

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

From a correctness standpoint, it might be better to use one record and one state hook. If we agree on that, I would like to apply that refactoring in a follow up checklist item.

</button>
</div>
</div>
</div>
Copy link
Author

@ghost ghost Mar 23, 2021

Choose a reason for hiding this comment

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

Start of new code

<div className="-mr-2 -my-2 md:hidden ">
<button
type_="button"
onClick=showMobileMenu
Copy link
Author

Choose a reason for hiding this comment

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

This was modified to add onClick

</span>
</a>
</nav>
</div>
Copy link
Author

Choose a reason for hiding this comment

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

I don't want to refactor the repetition above yet. There are two issues.

  1. The mobile menu should not list all entries from the regular menu, so I could loop over lists of entries, but that will be removed soon.
  2. I could create a small sub component for each link, but I would rather do that in one pass while refactoring all parts of this component.

@@ -41,205 +41,409 @@ type activatedEntry =
let make = (~content) => {
Copy link
Author

Choose a reason for hiding this comment

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

Only a small portion of these changes are real modifications. All the rest of the lines are a result of the source code formatter. I have indicated where code has changed below.

@ghost ghost requested a review from agarwal March 23, 2021 21:39
@ghost ghost marked this pull request as ready for review March 23, 2021 21:45
@ghost ghost mentioned this pull request Mar 23, 2021
6 tasks
@agarwal
Copy link
Member

agarwal commented Mar 23, 2021

I can't review the code tonight, but I did test functionality. Don't see any obvious problems. The menu opens and closes fine, and links from menu items work as expected.

@ghost
Copy link
Author

ghost commented Mar 23, 2021

Don't see any obvious problems.

I will merge so that other people can start using the menu now in mobile. We can create a follow-up branch based on PR review notes that emerge.

@ghost ghost merged commit 97630e3 into master Mar 23, 2021
@ghost ghost deleted the kw1/mobile-nav-flyout branch March 23, 2021 22:42
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant