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

refactor: add new mobile menu #151

Merged
merged 4 commits into from
Jul 24, 2021
Merged

refactor: add new mobile menu #151

merged 4 commits into from
Jul 24, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Jul 8, 2021

Since we are introducing new mobile UX in facebook/docusaurus#4273, we can already remove styles for old-responsive sidebar menu.

@slorber it's just not clear to me -- do we need to implement secondary menu functionality in Infima itself as before in menu.js file?

@lex111 lex111 requested a review from slorber July 8, 2021 22:36
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 8, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2021

Size Change: -208 B (0%)

Total Size: 539 kB

Filename Size Change
./packages/core/dist/css/default-dark/default-dark-rtl.css 78.1 kB -132 B (0%)
./packages/core/dist/css/default-dark/default-dark-rtl.min.css 54.4 kB -89 B (0%)
./packages/core/dist/css/default-dark/default-dark.css 78.1 kB -139 B (0%)
./packages/core/dist/css/default-dark/default-dark.min.css 54.4 kB -94 B (0%)
./packages/core/dist/css/default/default-rtl.css 77.2 kB -132 B (0%)
./packages/core/dist/css/default/default-rtl.min.css 53.9 kB -89 B (0%)
./packages/core/dist/css/default/default.css 77.2 kB -139 B (0%)
./packages/core/dist/css/default/default.min.css 53.8 kB -94 B (0%)
./packages/core/dist/js/menu.js 1.94 kB +199 B (+11%) ⚠️
./packages/core/dist/js/menu.min.js 1.94 kB +199 B (+11%) ⚠️
./packages/core/dist/js/navbar.js 1.08 kB +151 B (+16%) ⚠️
./packages/core/dist/js/navbar.min.js 1.08 kB +151 B (+16%) ⚠️
ℹ️ View Unchanged
Filename Size
./packages/core/dist/js/alerts.js 409 B
./packages/core/dist/js/alerts.min.js 409 B
./packages/core/dist/js/button-groups.js 267 B
./packages/core/dist/js/button-groups.min.js 267 B
./packages/core/dist/js/dropdowns.js 1.01 kB
./packages/core/dist/js/dropdowns.min.js 1.01 kB
./packages/core/dist/js/pills.js 270 B
./packages/core/dist/js/pills.min.js 270 B
./packages/core/dist/js/radio-behavior.js 705 B
./packages/core/dist/js/radio-behavior.min.js 705 B
./packages/core/dist/js/tabs.js 267 B
./packages/core/dist/js/tabs.min.js 267 B

compressed-size-action

@netlify
Copy link

netlify bot commented Jul 8, 2021

✔️ Deploy Preview for infima ready!

🔨 Explore the source changes: 652d0aa

🔍 Inspect the deploy log: https://app.netlify.com/sites/infima/deploys/60e86d1666ec6a0007a29111

😎 Browse the preview: https://deploy-preview-151--infima.netlify.app

@slorber
Copy link
Collaborator

slorber commented Jul 9, 2021

@slorber it's just not clear to me -- do we need to implement secondary menu functionality in Infima itself as before in menu.js file?

My position about Docusaurus<->Infima is that both are quite coupled and I don't think many users will use Infima outside Docusaurus users anyway.

I was in favor of moving it to the Docusaurus repo and making it an implementation detail of the classic theme, not necessarily recommend the usage of it for users, so that they could easily switch from one theme to another without re-implementing their pages.

You and @yangshun didn't really agree with this idea and wanted to keep it as a separate repo/project, so I don't have a strong opinion about which CSS rule should be in Infima or Docusaurus 😅 if it was me I'd just put everything in the Docusaurus repo and even rename Infima as docusaurus-theme-classic-css or something.

Now about the JS question, not 100% sure but it actually looks unused on Docusaurus, so it basically only has value for the demo page and the users of Infima outside Docusaurus. Not sure it's something we should invest in, as Docusaurus is unlikely to benefit from this.

I still see value in using some vanilla JS in Docusaurus: it would permit to mark some JS items as critical, and make them interactive before React hydration. But it's an effort that is not started yet and I'm not even sure it's worth investing in this, as React selective hydration and server components might make this optimization less useful.

@lex111
Copy link
Contributor Author

lex111 commented Jul 9, 2021

If we don't implement the secondary menu in Infima, then it doesn't make sense to include some styles for that, right?

For example, the code below is essentially needed for the secondary menu to work. And if this menu is not implemented in Infima itself, then there's not much point in related styles in the final bundle.

& > div {
flex-shrink: 0;
padding: 0.5rem;
width: calc(var(--ifm-navbar-sidebar-width));
}

Therefore, will implement secondary menu on Infima's demo page to follow the previously chosen approach. It also helps to keep secondary menu styles in one place (Infima), and in Docusaurus code we don't need to write any extra code for that. WDYT?

@slorber
Copy link
Collaborator

slorber commented Jul 9, 2021

In practice any CSS added to Infima is some additional dev friction for me so I'd rather have everything in Docusaurus.

If you really want to have more CSS in Infima, it's up to you and @yangshun, and I'll follow what you decide but it's not the choice I would make.

@lex111
Copy link
Contributor Author

lex111 commented Jul 9, 2021

I just like the consistency, so I think it makes sense to stick with the previous approach.

@github-actions
Copy link

github-actions bot commented Jul 9, 2021

Dist CSS Diff

diff --git a/packages/core/dist/css/default/default.css b/packages/core/dist-branch/css/default/default.css
index 7d4c397..a7341bc 100644
--- a/packages/core/dist/css/default/default.css
+++ b/packages/core/dist-branch/css/default/default.css
@@ -2392,33 +2392,6 @@ hr:after {
         background: var(--ifm-menu-color-background-active);
       }
 
-.menu--responsive .menu__button {
-      bottom: 2rem;
-      display: none;
-      position: fixed;
-      right: 1rem;
-      z-index: var(--ifm-z-index-fixed);
-    }
-
-.menu--show {
-    background: var(--ifm-background-surface-color);
-    bottom: 0;
-    left: 0;
-    -ms-scroll-chaining: none;
-        overscroll-behavior: contain;
-    padding: 1rem;
-    position: fixed;
-    right: 0;
-    top: 0;
-    z-index: var(--ifm-z-index-overlay);
-  }
-
-.menu--show .menu__list {
-      display: inherit;
-      opacity: 1;
-      transition: opacity var(--ifm-transition-fast) linear;
-    }
-
 /**
  * Copyright (c) Facebook, Inc. and its affiliates.
  *
@@ -2657,14 +2630,38 @@ html[data-theme='dark'],
 .navbar-sidebar__items {
       display: flex;
       height: calc(100% - var(--ifm-navbar-height));
+      transform: translateZ(0);
+      transition: transform var(--ifm-transition-fast) ease-in-out;
+    }
+
+.navbar-sidebar__items--show-secondary {
+        transform: translate3d(
+          calc((var(--ifm-navbar-sidebar-width)) * -1),
+          0,
+          0
+        );
+      }
+
+.navbar-sidebar__item {
+      flex-shrink: 0;
+      padding: 0.5rem;
+      width: calc(var(--ifm-navbar-sidebar-width));
     }
 
-.navbar-sidebar__items > div {
-        flex-shrink: 0;
-        padding: 0.5rem;
-        width: calc(var(--ifm-navbar-sidebar-width));
+.navbar-sidebar__item--secondary {
+        padding-top: 0;
       }
 
+.navbar-sidebar__back {
+      background: var(--ifm-menu-color-background-active);
+      font-size: 15px;
+      font-weight: var(--ifm-button-font-weight);
+      margin: 0 0 0.7rem -0.5rem;
+      padding: 0.6rem 1.5rem;
+      text-align: left;
+      width: calc(100% + 1rem);
+    }
+
 /**
  * Copyright (c) Facebook, Inc. and its affiliates.
  *
@@ -2971,14 +2968,6 @@ html[data-theme='dark'] {
     padding-right: 0
 }
 
-.menu--responsive .menu__button {
-        display: inherit
-    }
-      .menu--responsive:not(.menu--show) .menu__list {
-        display: none;
-        opacity: 0;
-      }
-
 .navbar > .container,
   .navbar > .container-fluid {
       padding: 0

@lex111 lex111 changed the title refactor: remove responsive (old) menu refactor: add new mobile menu Jul 10, 2021
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

That looks fine to me.
Just afraid it would add dev friction it we later need to do additional changes.
For example I think we could later replace the back arrow with a proper svg or something, and this would probably require changing Infima again. I'm not too fan of these back-and-forth due to having 2 repos instead of 1.

If you think we should merge it, feel free to do so and cleanup on Docusaurus side

@lex111
Copy link
Contributor Author

lex111 commented Jul 24, 2021

Just afraid it would add dev friction it we later need to do additional changes.

Do you mean that we will need to make changes to appropriate React component in Docusaurus? Yes, of course, will prepare PR for this.

For example I think we could later replace the back arrow with a proper svg or something, and this would probably require changing Infima again. I'm not too fan of these back-and-forth due to having 2 repos instead of 1.

This can be done without changing Infima code, here we just provide basic styles to make two menus work.

@lex111 lex111 merged commit efebbba into master Jul 24, 2021
@lex111 lex111 deleted the lex111/remove-res-menu branch July 24, 2021 15:45
@slorber
Copy link
Collaborator

slorber commented Jul 27, 2021

Do you mean that we will need to make changes to appropriate React component in Docusaurus? Yes, of course, will prepare PR for this.

I mean that having to release Infima and then Docusaurus adds DX friction. If we were able to modify Infima + Docusaurus and see the live result of those changes in a single atomic PR, that would be much more convenient to review and ensure that the Infima side + Docusaurus side are both compatible with each other.

A scenario that happens from time to time is that you change Infima, release it, then try to integrate the changes on Docusaurus, figure out Infima changes were not good enough, have to release Infima again with new changes to only then have everything working in Docusaurus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants