Fat menu and mobile menu implementation. #237
Conversation
|
@sneridagh Ready for first review from my side. I tried to fix as much code as possible, but please review css code. Also I am not able to fix the fatmenu in Edit mode. I don't want to write css for specific cases. Please try it once locally and review it. |
There was a problem hiding this comment.
@iFlameing I can see the menu a bit shifted up:
Regarding the edit, I won't display it, just disable it. There's no point to display it in there.
|
After :hover 
After :click 
|
|
@sneridagh I don't know why I am getting these linting problem in my new laptop. I ran the make format still I am getting error. |
8a2ca0b to
f1f1026
Compare
|
@iRohitSingh I had to fix the height of the overall header was a bit off, that's why it wasn't looking well (I know only a few pixels), but still... but now the menu and the underline are a bit off again 🙂 could you please fix it? |
|
@sneridagh Fixed. 
Add page. |
| function toggleMobileMenu() { | ||
| const body = document.getElementsByTagName('body')[0]; | ||
| if (!isMobileMenuOpen) { | ||
| body.style.position = 'fixed'; |
There was a problem hiding this comment.
@iFlameing Is there really no way around inline styling? I know this is the kind of styling we likely won't ever touch/overwrite for the mobile navigation but .. we should still try to avoid it.
There was a problem hiding this comment.
There are no other options than this. We want this behavior for the mobile menu and we can only do it by js manipulation. Before react you not only do for style but also for appending div and section also. Just think how cumbersome it was before.
There was a problem hiding this comment.
You can also inject a className ;) I think that is what I was doing in zeelandia and other projects.
|
| padding-top: 20px; | ||
| padding-bottom: 6px; | ||
| border: none; | ||
| border-bottom: 9px solid transparent; |
There was a problem hiding this comment.
@iFlameing @sneridagh @iRohitSingh why do we need this transparent border? Couldn't we instead set https://github.com/kitconcept/volto-light-theme/blame/bcb4262a54d1c51aba83ba552c70d25b34f535c3/src/theme/_header.scss#L287 to -40px?
There was a problem hiding this comment.
@danalvrz As you can clearly see if we don't give the border 9px transparent we wll not get the desired the space between navigation and fatmenu which is 35px according to figma.
Also if you see submenu-wrapper has a marign-top 40px. So if you don't give the border 9px then we will be short of space.
You are right if we don't want this transparent border we have to do these changes.
- Give submenu-wrapper margin 49px.
- remove the border-bottom
- make it 40px before.
But there is also other region see this screenshot
this is example of demo plone org. we use the the border bottom to show a active one. I think this is why we use that. I am just giving you cases that how it landed in there. Not a correct description why we did that in first place.
There was a problem hiding this comment.
@iRohitSingh thanks for the answer! In general, creating spacing with transparent lines seems odd, but I get the use case here. Maybe we can revisit this at some point in volto, and change the approach.
On purpose I left back content in mobile menu because it make much more sense. All the visaully implementation are there. I need to refactor the code. so please don't review the code only look and feel and ui implementation please.
cc @sneridagh @davisagli