OdyseeTeam / odysee-frontend

The code that runs odysee.com
https://odysee.com
MIT License
213 stars 68 forks source link

Route-leading menu items aren't proper HTML hyperlinks #190

Closed vintprox closed 2 years ago

vintprox commented 2 years ago

Bug Links seen in navigation menus cannot be open in a new tab. It is because they are not even semantically hyperlinks.

To Reproduce Steps to reproduce the behavior:

  1. Press any button in the header navigation that opens up a menu list (one of these: publish button, settings, profile picture).
  2. Try middleclicking any item in popped up menu.
  3. See that it doesn't open in a new tab.
  4. Repeat 1.
  5. Try dragging list item in the tab drawer.
  6. See no response.
  7. Press up/down arrows to focus on item.
  8. Try pressing Ctrl + Enter.
  9. See that it doesn't create a new tab.
  10. Feel inferior, because now you have to duplicate tab to save what you were seeing in the first one 😂

Expected behavior Menu items that lead to pages of multi-page application are expected to act as normal HTML hyperlinks. Item's page should open in a new tab when user presses mousewheel (mouse navigation) on item or when, also retaining focus on it, user presses Ctrl + Enter (keyboard navigation). Being the normal hyperlink, it should also be imperative to open in a new tab by just dragging it in the tab drawer.

Screenshots image

System Configuration

Additional context Guess, I can solve it. Just need some free time.

vintprox commented 2 years ago

So, I located the file responsible for header navigation: view.jsx. There is no MenuLink component used, but only MenuItem for all these supposed links.

I tried converting "Settings" item from this:

          <MenuItem className="menu__link" onSelect={() => history.push(`/$/${PAGES.SETTINGS}`)}>
            <Icon aria-hidden tooltip icon={ICONS.SETTINGS} />
            {__('Settings')}
          </MenuItem>

to this:

          <MenuLink className="menu__link" as={Link} to={`/$/${PAGES.SETTINGS}`}>
            <Icon aria-hidden tooltip icon={ICONS.SETTINGS} />
            {__('Settings')}
          </MenuLink>

which requires two more things: Link from React Router, and MenuLink from Reach library.


import { withRouter, Link } from 'react-router';
...
import { Menu, MenuList, MenuButton, MenuItem, MenuLink } from '@reach/menu-button';

The problem is that to isn't getting recognized as a prop for underlying Link, instead it's passed as a generic attribute: image

According to this example and my feeling it should have been a functioning hyperlink. Although, the documentation is really unclear with drawing expectations...

Need help with making it work.

vintprox commented 2 years ago

I see now. I needed to import Link from react-router-dom, not react-router. What a hot mess 🥱

Now my solution works. Shall publish the PR for header navigation soon.