Fix mobile doc sidebar #1279
Fix mobile doc sidebar #1279
Conversation
|
|
| @@ -60,10 +60,6 @@ | |||
|
|
|||
| &:hover { | |||
| color: #3c3937; | |||
|
|
|||
| @media (--xs-scr) { | |||
There was a problem hiding this comment.
@iKBAHT why do we remove this?
There was a problem hiding this comment.
This fixed bug.
Steps:
- Open docs in mobile
- Open sidebar
- Click to "Install"
Exp: "Install" is highlighted
Act: "Install" remains as it was
There was a problem hiding this comment.
Not an easy to test in the preview environment for me (your change - this one removal) to see the bug you mention. But this one removal introduces another one for me - when I move the sidebar with my finger it highlights (and keep it highlighted) any item under my finger. So, you kinda see two items highlighted at once.
There was a problem hiding this comment.
Sorry, it is my fail. I fix this and fix the bug that I found.
| @@ -40,6 +40,8 @@ const SidebarMenuItem: React.FC<ISidebarMenuItemProps> = ({ | |||
| const isActive = activePaths && includes(activePaths, path) | |||
| const isRootParent = | |||
| activePaths && activePaths.length > 1 && activePaths[0] === path | |||
| const isLeafItem = children === undefined || children.length === 0 | |||
There was a problem hiding this comment.
(not sure we need to change or fix this in this PR) curious - is it undefined or zero length array after all? this check feels a bit excessive.
There was a problem hiding this comment.
This isn't currently necessary as no sidebar items return empty arrays (and probably won't as long as it's defined as JSON). On top of that, if the issue ever arises we can control for that kind of thing at build time.
Removing this extra check is a micro-optimization, but a valid one nonetheless.
|
I'll run this as well, but simply looking at the code doesn't make any issues jump out to me. I'll also try to address @shcheklein's comments. |
There was a problem hiding this comment.
The code looks fine to me. The zero-length array check @shcheklein pointed out is benign but unnecessary right now, so I'll leave it to him to decide if we want to approve this now or after that is addressed. I'd also like to know why that bit of CSS was removed.
Besides that, I approve. Thanks for the fix!
iKBAHT
force-pushed
the
fix-mobile-sidebar
branch
from
47719aa to
98ec846
Compare
|
Rebased from the master |
Fixes #808
There is some conflict in the mobile doc sidebar. Some root sidebar items have their own articles. When the user clicks to that item two things happen:
On mobile this cause problem because both logic can't be visible at the same time. I think a better way to solve the issue is to separate 1 and 2 logic to different clicks (ie need special UI element to collapse submenu). To do it need some other design for a mobile sidebar.
As a quick fix, I implement the following logic: the mobile doc sidebar doesn't close on navigating until leaf item is selected.