add option enable/disable responsive menu

[UseMuse]

I have a page with a lot of data. When I flip the phone to the horizontal position, I would not want the menu to open automatically. To do this, I propose to add an add-on over this

[UseMuse]

[UseMuse]

How can I quickly disable the auto menu opening?

[TarikHuber]

We could make it optional 🤔 Give some time and I will build it in. Or do you want to give it a try?

[UseMuse]

@TarikHuber better you, i don't know how to do it

[jswelsh]

@UseMuse to clarify a few things. This is before me cracking open the code to see what really is going on, but from me playing around with this myself, I notice a few things (even a separate bug I think).

First, I think your issue is this

• By default the sidebar state is open by default
• When the the default breakpoint greater than sm, then transition to less than sm, the state is maintained as open, but it's conditionally not render.

this is reasonable behaviour for UX. Lets say the user continues using the app and forgets they had the menu open, finally they increase their window to something larger than sm, to view some content and automatically the sidebar auto opens when the breakpoint is greater than sm. I can see why this is jarring and may not be the best UX.

a fix would be to instead of just conditionally rendering the sidebar open depending on breakpoints, but SETTING the sidebar state to being closed once the breakpoint threshold ever becomes sm. This may not be best, but I think has merits. If this is in fact what you want I think is a simple fix.I just want to make sure that's your desired functionality.

Keep in mind, the menu by default is open, so while using a small screen, the user may not realise the state of the sidebar but it IS open just conditionally not rendered.

A quick solution is setting the default state for the sidebar to being closed which I think is the easiest and actually the better solution. @TarikHuber Thoughts?

on a side note you can completely break the originally intended behaviour if you open the sidebar in breakpoint less than sm causing the conditional not to function when you try to close it again you can break it further by closing the sidebar on a breakpoint larger than sm causing an inverse of the intended behaviour. I will fix these.

[TarikHuber]

@jswelsh you can solve it as you want. I did it that way because sometimes users cam into a state where they could not use the menu withour resizing the screen. The main reson I have it initial open is that the PWA is in our production apps mostly used on mobile devices and much smaller screens.

[UseMuse]

@TarikHuber you need to expand the application settings by adding the ability to disable/enable auto menu expansion. I hope you can do it

[jswelsh]

I had sometime to think about the problem and look at the code. I haven't done much mobile friendly development so keep that in mind.

the quick fix for @UseMuse would be setting to prevent the menu state being open as default:

replacing const [isDesktopOpen, setDesktopOpen] = useState(true) with const [isDesktopOpen, setDesktopOpen] = useState(false) OR keeps the menu open, but in the in the mini state, to address @TarikHuber original intended user UX, but still prevents the overly jarring experience of the full menu opening when a screen size is widened as @UseMuse is experiencing replacing const [isDesktopOpen, setDesktopOpen] = useState(true) with const [isDesktopOpen, setDesktopOpen] = useState(false) replacingconst [isMini, setMini] = useState(false) with const [isMini, setMini] = useState(true)

In mainAppFolder/material-ui-shell/src/providers/Menu/Provider.js

To fix this problem completely, the edge case bugs need to be addressed I noticed, in local storage we have state for

• isMiniKey
• isUseMiniModeKey
• isDesktopKey // isDesktopOpen

isMobileOpen is not kept there

• you can toggle isDesktopOpen's boolean and it wont effect isMobileOpen
• you can toggle isMobileOpen's boolean and it WILL effect isDesktopOpen
• you can go from a isDesktopOpen being true to isMobileOpen being false ( This is good, and intended behaviour )
• you can go from a isMobileOpen being true to isDesktopOpen being false ( I'm not sure if I like this behaviour)
• you can go from isDesktopOpen being false to isMobileOpen being true ( also not ideal)

I think we could have one state as an object menu: { isOpen: boolean, mode: string //either desktop, mobile or mini }

this would get rid of a bunch of redundant state and complexity in having to toggle the state of the menu, I feel trying to track down the state in local and context with this many different states maybe more then necessary. this will require a fair bit of refactoring.

for now I think the better fix is creating a toggle function that checks if there is a value: boolean (force-setting both mobile and desktop menu's "opennes" to being true or false depending on the use case) if no value exists, toggle both isMobileOpen, isDesktopOpen current state using !currentState . Pretty sure this could maintain continuity from transitioning from sm -> lg screen width and vice verse while still keeping the autoclose when going from large screen to small.

I will work on this and should have it done within day, but any thoughts on this are welcome .

[TarikHuber]

Can't agree more. If you want you can simplify the state management with those two variables. We can use the configs to get the default value from there. That way we can adjust the default behaviour without anyone changing the code.

[jswelsh]

Old behaviour

---

New behaviour

---

@TarikHuber If the new behaviour looks acceptable I can push the code up. There's still work that needs to be done in consolidating the state into a single object, but this will fix the main issues until more refactoring is done.

some notes on what I'm adding

add toggler for the state for both mobile and desktop

  const toggleClosedMenu = (setTo) => { 
      setDesktopOpen(setTo || !isDesktopOpen)
      setMobileOpen(setTo || !isMobileOpen)
  }

A useEffect to check if the user resizes window

  useEffect(() => {
    const debouncedHandleResize = debounce(() => {
      //if the VW is resized to sm then toggle all menus closed
      if(window.innerWidth <= 600) {
        toggleClosedMenu(false)
        setMini(false)
      }
    }, 1000)
    window.addEventListener('resize', debouncedHandleResize)
    return _ => {
      window.removeEventListener('resize', debouncedHandleResize)
    }
  })

add debounce on checking the window being resized, set to 1 second, maybe I should increase the delay?

  function debounce(callback, ms) {
    let timer
    return () => {
      clearTimeout(timer)
      timer = setTimeout(() => {
        timer = null
        callback.apply(this, arguments)
      }, ms)
    }
  }

I know having a listener may not be ideal, but I think with the debounce it should be ok.

as far as setting up the code to be programatically changed depending on settings used on the CLI when first installing is beyond my current knowledge, so I'm not sure how to implement that.

[UseMuse]

I think the solution should be simpler

[jswelsh]

I agree to some degree, the complexity I feel is rooted in the multiple different states of isDesktopOpen, isMobileOpen, useMiniMode, isMini being changed all separately. It is made a bit more difficult with the side menu openness is dependent on state BUT can be opened without changing the state through the Responsive Drawer that uses the property variant value temporary

the issue you are facing I think can easily be solved my setting the starting state for the menu to being closed. having an option for responsive menu or not doesn't change the fact that if the responsive menu is chosen, the other bugs still exist. These changes are to address the other bugs which are related to your issue. I don't think they can be solved unless the different menus state are coupled through some form of toggling function or consolidating the state into a single object that we spread the state into and then update. currently we have

  const [isDesktopOpen, setDesktopOpen] = useState(false) 
  const [isMobileOpen, setMobileOpen] = useState(false)
  const [useMiniMode, setMiniMode] = useState(useMini)
  const [isAuthMenuOpen, setAuthMenuOpen] = useState(false)
  const [isMini, setMini] = useState(false) 

the new state would look something like

const [menu, setMenu] = useState({
mode: string,
isOpen: boolean,
isAuthenticated: boolean, 
})

changing the state would look like this

setMenu({
...menu, 
open:true
})

or

setMenu({
...menu,
mode:desktop,
open:false,
})

I feel this is more concise, we can remove code located in a few other files, while making the code easier to reason with in the long run. I may be way off on solving your issue and the bugs but I really can't think of a couple-liner/silver bullet

[TarikHuber]

@jswelsh can't we use the Material-UI mediaQuery. That way we don't need to care a bout browserspecific bugfixing.

Also if you want to have the state in a single object I would recommend to make the state changes with a useReducer.

Using useState can cause problems if some events overlap in time. Only one of them will be saved. The useReducer ensures that all of them are saved as expected.

[TarikHuber]

@jswelsh thx for the PR Could you pls fix the user menu when on mini mode

it should be like this

[TarikHuber]

@jswelsh could you pls comare your version of the provider and the one I commited.

You don't need a eventlistener for resize. isDesktop will change on resize and putting it in the useEffect dependencies will rerender the whole menu with the new state.

You also saved the current state to localStorage but never loaded it to for initial state. I saw a useEffect that should do that bit it did not work. If you check out the way I did it you can notice much less code but same result.

You did it great! As more you work on this project you will see more ways how to do it easier.

[UseMuse]

@TarikHuber and @jswelsh since the menu is busy, I suggest adding the option "enable/disable breadcrumbs for the menu" in the application settings, when turned on, when you click on a link, for example https://www.react-most-wanted.com/companies in the menu assumed the display state of this route

now

expecting

[jswelsh]

how do you even get to that page without manually typing in the address, there isn't a button for that route yet is there? I can't find it atleast.

[jswelsh]

@UseMuse , Thought about it a bit, would you prefer something like this? You can click any one of the breadcrumbs, Home / Demos which is linked to the respective route.

just spit balling.

[UseMuse]

@jswelsh I didn't mean it, but that the menu should be selected according to the route in the link

[jswelsh]

@UseMuse I'm unfortunately confused on the issue.

The current default behaviour has the selected route button toggled to being background-color: #fafafa. In the following example it's Tasks and its parent Demos being separated by a <Divider /> correct?

In your expected example you show the same behaviour, just with a different route (Companies). correct? If that is the case then isn't the issue that some of the routes aren't connected properly to the routing tree like this.

Home
└───Demos
│   │   Companies // needs to be added, here or to another node
│   │   Admin
│   │   Tasks
│   │   Posts
│  
└───Documentation
│   │   Getting started
│
└───..................

with associated buttons in the menu.

Sorry, I just need a bit more clarification on this.

[UseMuse]

@jswelsh when you click on any link (link, not using the menu. Suppose you dropped the link and you followed it), for example: https://www.react-most-wanted.com/companies https://www.react-most-wanted.com/tasks https://www.react-most-wanted.com/posts the menu is always static но я это вижу по другому, мне кажется, что лучше бы при переходе по ссылке меню выглядело бы так, как если бы меню было нажато, а не по ссылке

[jswelsh]

Right I see what you mean now. That makes sense, I agree.

[UseMuse]

@jswelsh I think this menu behavior needs to be made optionally configurable from the application settings

[jswelsh]

The way I see it is import { Switch, useLocation } from 'react-router-dom' const location = useLocation() {location.pathname} is simple enough, but to get the effect you want, wouldn't we need to change the routing to include the parent directory aswell? so changes in routes.js would go from

/dialog_demo"
/toast_demo"
/filter_demo"
/list_page_demo"

to

demos/dialog_demo"
demos/toast_demo"
demos/filter_demo"
demos/list_page_demo"

and other files would have to reflect these changes. With this, we are able to grab the parent and child to conditionally style the menu based on the users URL, assuming the settings in the config file reflect that. Does this seem right?

[TarikHuber]

@jswelsh what do you think about adding RTL to the menu?

[jswelsh]

never done RTL support, but should be easy enough. I'll look into best practices, though I assume we just want the menu to be right-justified, setting up an option in the config file to opt-in to that mode.

[TarikHuber]

@jswelsh yes just moving the menu to the right and orientating the icons to make sense (arrow icons). The language should be made RTL by the react-inlt automaticaly. Maby take a look at the MUI documenation. I think they have a demo for RTL drawer menu.

[jswelsh]

@TarikHuber I have time this weekend to commit to this. One Quick question on implementation. Do we also want a toggle button in the MenuHeader similar to MUI documentation? maybe have it nested in settings, as to not clutter the interface? or would that be redundant/pointless. The P tag being the icon for toggling

[TarikHuber]

Hi @jswelsh , we could add it to the MenuHeader with the option to not show it if set so in settings. The "miniMenu" button is also made like that. I usualy disable it in production but it's nice to have it in a demo like RMW just to demonstrate that it works and how.

[jswelsh]

hey, @TarikHuber I think I may need a bit of help. I got the basic functionality when I hard code style={{ direction: "theme.direction" }} in the MUI/src/containers/appContainer.js

import React from 'react'
import MenuProvider from 'material-ui-shell/lib/providers/Menu/Provider'
import ThemeProvider from 'material-ui-shell/lib/providers/Theme/Provider'
import { useConfig } from 'base-shell/lib/providers/Config'
import { useTheme } from '@material-ui/core/styles'
import { useTheme as useAppTheme } from 'material-ui-shell/lib/providers/Theme'

import { create } from 'jss';
import rtl from 'jss-rtl';
import { StylesProvider, jssPreset } from '@material-ui/core/styles';
// Configure JSS
const jss = create({ plugins: [...jssPreset().plugins, rtl()] });

export default function ({ children }) {
  const { appConfig } = useConfig()
  const theme = useTheme()
  const { direction } = useAppTheme()

// console.log("in appContainer direction:", direction)
// console.log("in appContainer, direction:", theme.direction)
  return (
    <React.Fragment>
    <StylesProvider jss={jss}>
      <MenuProvider appConfig={appConfig}>
        <ThemeProvider appConfig={appConfig}>
          <div
            style={{
              display: 'flex',
              //direction: theme.direction //doesn't work
              //direction: direction //doesn't work
               direction: "rtl" //works

            }}
          >
            {children}
          </div>
        </ThemeProvider>
      </MenuProvider>
    </StylesProvider>
    </React.Fragment>
  )
}

but when I try to set the direction as state in the MUI/src/providers/Theme/Provider.js and change it via a toggle, it isn't triggering a rerender on appContainer.js. when used in other components it rerenders just fine. I assume you'll need to check the code but maybe you know right away why the appContainer.js is instantiated once and not rerendered.

[TarikHuber]

@jswelsh can you show me the code you use in the toggle to change the state?

You should to all that stuff in the ThemeProvider. Check out how I switch between dark and light theme. using:

const [type, setType] = useState(defaultType)

With that provider just make a state for RTL and a setRTL function that you can use from anywhere in avery app.

You will probably also need to change the getThemeSource in the LayoutContainer to take a last param witch is default RTL disabled like:

const theme = getThemeSource(themeID, themes, type, isRTL)

Let me know if you need more help.

[jswelsh]

these are what I have for testing, I was going to move the toggling into the settings menu once I got this working, but commented out the light/dark mode toggler and placed the toggle there for now. I tried to follow the patterns you used for dark mode and I'm pretty sure everything is set up right, the issue is the appContainer.js only triggers the first render and never rerenders. This is where the highest <div> needs to have the current selected direction property set.

../material-ui-shell/src/components/MenuHeader/MenuHeader.js

//all imports
//added
import FormatTextdirectionRToLIcon from '@material-ui/icons/FormatTextdirectionRToL'
import FormatTextdirectionLToRIcon from '@material-ui/icons/FormatTextdirectionLToR';
// import { useTheme as useAppTheme } from 'material-ui-shell/lib/providers/Theme'//add

//useStyle code

const MenuHeader = () => {
  const theme = useTheme()
  const { auth } = useAuth()
  const { type, setType, setDirection ,direction } = useAppTheme()
  const authData = auth
  const classes = useStyles()
  const {
    isDesktop,
    setMenuOpen,
    setMiniMode,
    isMiniMode,
    isMenuOpen,
    isMiniSwitchVisibility,
    isAuthMenuOpen,
    setAuthMenuOpen,
  } = useMenu()
console.log("in menuHeader direction:",direction)
  const isAuthenticated = auth.isAuthenticated
  const AvatarConstructor = ({src, alt, avatar}) => {
    return (
    <ListItemAvatar
      onClick={() => setAuthMenuOpen(!isAuthMenuOpen)}>
      <Avatar src={src} alt={alt}> {avatar} </Avatar>
    </ListItemAvatar>
    )
  }

  return (
    <Paper square={true} className={classes.paper}>
      // some code...
      //this is added and replaced the dark mode toggler
              <ListItemSecondaryAction>
                <IconButton
                  onClick={() => {
                    setDirection(direction === 'ltr' ? 'rtl' : 'ltr')
                  }}>
                  {direction === 'ltr' && <FormatTextdirectionLToRIcon />}
                  {direction === 'rtl' && <FormatTextdirectionRToLIcon />}
                </IconButton>
           //more code
           //added this
                      {theme.direction === 'rtl' && (
                        <ChevronRight classes={{ root: classes.icon }} />)}
                      {theme.direction !== 'rtl' && (
                        <ChevronLeft classes={{ root: classes.icon }} />)}
                    </IconButton>{' '}
                  </>
                )}
              </ListItemSecondaryAction>
          </ListItem>
        )}

       //rest of code 
      </List>
    </Paper>
  )
}

export default MenuHeader

../material-ui-shell/src/providers/Theme/Provider.js

import PropTypes from 'prop-types'
import React, { useState, useEffect } from 'react'
import Context from './Context'

const Provider = ({ children, persistKey = 'theme', appConfig }) => {
  const { theme: themeConfig } = appConfig || {}
  const { defaultThemeID, defaultType, defaultDirection } = themeConfig || {}
  const [themeID, setThemeID] = useState(defaultThemeID)
  const [type, setType] = useState(defaultType)
  const [direction, setDirection] = useState(defaultDirection)//add

  const themeIDKey = `${persistKey}:themeID`
  const typeKey = `${persistKey}:type`
  const directionKey = `${persistKey}:direction`//add

  useEffect(() => {
    const persistThemeID = localStorage.getItem(themeIDKey)
    const persistType = localStorage.getItem(typeKey)
    const persistDirection = localStorage.getItem(directionKey)//add

    if (persistThemeID) {
      setThemeID(persistThemeID)
    }
    if (persistType) {
      setType(persistType)
    }
    if (persistDirection) {
      setDirection(persistDirection)
    }
  }, [themeIDKey,typeKey,directionKey])

  useEffect(() => {
    try {
      localStorage.setItem(themeIDKey, themeID)
    } catch (error) {
      console.warn(error)
    }
  }, [themeID,themeIDKey])

  useEffect(() => {
    try {
      localStorage.setItem(typeKey, type)
    } catch (error) {
      console.warn(error)
    }
  }, [type,typeKey])

  useEffect(() => {
    try {
      localStorage.setItem(directionKey, direction)
    } catch (error) {
      console.warn(error)
    }
  }, [direction,directionKey])//add

  return (
    <Context.Provider
      value={{
        themeID,
        type,
        setThemeID,
        setType,
        direction,//add
        setDirection//addd
      }}
    >
      {children}
    </Context.Provider>
  )
}

Provider.propTypes = {
  children: PropTypes.any,
}

export default Provider

../material-ui-shell/src/utils/theme.js

import { createMuiTheme } from '@material-ui/core/styles'

const getThemeSource = (id, ts, type = 'light', direction) => {
  console.log("in theme, direction:", direction);
  if (ts) {
    for (let i = 0; i < ts.length; i++) {
      if (ts[i]['id'] === id) {
        const source = ts[i]['source']
        const palette = source != null ? source.palette : {}
        return createMuiTheme({
          ...source,
          palette: { ...palette, type },
          direction: direction
        })
      }
    }
  }

  return createMuiTheme({
    palette: { type },
    direction: direction
  })
}

export default getThemeSource

[TarikHuber]

Does it need to be the highest div? The Theme and all the rest starts in the LayoutContainer and not AppContainer. The AppContainer ist just there to combine the App with the LandingPage and share configs and other global state. The AppContainer also has no UI at all.

[jswelsh]

from MUI RTL Doc

1. HTML Make sure the dir attribute is set on the body, otherwise native components will break:

when using google chrome dev tools, I manually inject CSS values into HTML Elements to see what will work and what won't. The only thing that makes everything work is when I have the direction property set on the <div> in the appContainer OR the <body> tag.

The appContainer's <div> is the highest tag before the body tag.

I'm going to try a couple of things, your previous recommendations make me confident that the code I have already is probably correct and that I'm almost there.

[TarikHuber]

Try to replace the div in AppContainer with a React Fragment 🤔 then the one in LayoutContainer should be the top one

[jswelsh]

@TarikHuber you nailed it! :partying_face:

Now I just need to clean up my spaghetti code... thanks for the feedback.

[TarikHuber]

thx @jswelsh . Works great 😄