Update header and footer to use MUI #14
Conversation
|
✔️ Deploy Preview for orcahome ready! 🔨 Explore the source changes: 1921cf4 🔍 Inspect the deploy log: https://app.netlify.com/sites/orcahome/deploys/61e285e9385d9d00082ee58e 😎 Browse the preview: https://deploy-preview-14--orcahome.netlify.app |
| "@material-ui/styles": "^4.11.4", | ||
| "@mui/icons-material": "^5.2.1", | ||
| "@mui/material": "^5.2.6", | ||
| "@mui/styles": "^5.2.3", | ||
| "bootstrap": "^4.6.0", |
There was a problem hiding this comment.
I think we can remove this dependency as well.
There was a problem hiding this comment.
I think we should keep this for now as other parts of the app are still using bootstrap.
There was a problem hiding this comment.
Looks pretty good @DivyanshuParwal, thanks for your work and patience! I left some comments for you as feedback but nothing critical. Let's not worry about fixing them right now because I want to get things merged in ASAP. Can always make them into additional issues.
Two more things I want to add:
- Ideally the MUI setup should be its own PR, but will merge as is
- For some reason there seem to be some empty files, no worries I'll remove them later
| </AppBar> | ||
| </Box> | ||
| ) | ||
| return ( |
There was a problem hiding this comment.
It would be slightly cleaner to have this be either an if/else or ternary, especially if the mobile and desktop JSX are moved into their own variables/components. Something like what I ended up doing in the orcasite repo
| {iconContainer} | ||
| <Button | ||
| variant="contained" | ||
| sx={{ |
| borderRadius: '100px', | ||
| '&:hover': { color: 'black', backgroundColor: 'white' }, | ||
| }} | ||
| startIcon={<NotificationsIcon sx={{ color: '#F79234' }} />} |
There was a problem hiding this comment.
I would try to not use one-off colors and try to set everything up in the theme instead
|
|
||
| const useCheckMobileScreen = () => { | ||
| const [width, setWidth] = useState(0) | ||
| const handleWindowSizeChange = () => { |
There was a problem hiding this comment.
This is one way to check mobile, might be easier just to use the media query hook that comes with MUI. Check out how I did it in orcasite
| }) | ||
|
|
||
| const Nav = () => { | ||
| const [anchorElNav, setAnchorElNav] = React.useState(null) |
There was a problem hiding this comment.
This works fine but it might be simpler just to store open state as boolean and pass the anchor ref in directly (since it's always the same icon)
paulcretu
changed the title
|
As I send design to production, how can I document that the header and footer nav bars are persistent throughout the entire Orcasound web experience? Where do I point devs to implement the header and footer nav bars? It would be best to have a single header and nav bar implementation, so that when changes are made, it updates throughout the Orcasound web experience. |
UXBrendan
added
type: documentation
|
At the end of the day it's up to the devs who are implementing the nav bars. The way it's set up right now there are components ( I would make it clear that "consistent nav bars" is a requirement for getting PRs approved/merged. As for keeping nav consistent across orcahome and orcasite, easiest is to copy changes over manually (eventually we could share components but it's more complicated). I would wait to cross that bridge once we get there (orcahome and orcasite v3 are close to done). I'm intentionally focusing on nav last for orcasite. Also it's not that hard to modify, so we can adjust as we go. |
No description provided.