Skip to content

Refactor Nav pt. 2 - create sub-components #2227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 10, 2023

Conversation

lindapaiste
Copy link
Collaborator

Fixes #2224

Extends PR #2210

Changes:

This PR should not change any UI or behavior, with one tiny exception: I removed a few title= tags which were causing unhelpful tooltips (see also #2150, #1266).

I'm changing how the UI is generated by breaking it up into various building blocks that can be put together to form the current Nav. I'm cutting pieces of code out of Nav and pasting them into new files. In the best cases, I'm cutting code that's repeated in multiple spots and pasting it only to one usage.

New Components

NavBar

The NavBar is the parent that wraps all of the elements in the navigation.

  • Controls the state of which dropdown is currently open.
  • Contains all existing timeout logic from Nav.
  • Registers event listeners for close on Esc and close on outside click behavior.
  • Generates event handlers for top-level dropdowns and individual menu items, and makes them available via context.
  • Provides a context of which menu is open.
  • Renders the header and nav elements.

NavDropdownMenu

The NavDropdownMenu is a top-level item in the nav bar which contains a submenu (File, Edit, etc.).

  • Gets its event handlers via context.
  • Knows if it is open via context.
  • Provides its own id to its children via context.
  • Renders the UI for the button including the title and the triangle icon, and a dropdown list containing its children.

NavMenuItem

The NavMenuItem is for each item a dropdown menu.

  • Gets its event handlers via context.
  • Wraps in an li with the correct className (likely to be changed to a styled component in the future).
  • Has a prop hideIf for cleaner conditional rendering.
  • Basically a wrapper around ButtonOrLink.

ButtonOrLink

I put this in the common folder because it can be used in other places. ButtonOrLink is a helper that allows buttons and links to be used interchangeably.

  • Renders a button when there is no href prop.
  • If the href is an external link, it renders an a and automatically adds target="_blank" rel="noopener noreferrer".
  • If the href is an internal link, it renders a react-router Link.

Changes to Nav

  • For each dropdown menu, delete a block of code that looks like this:
<li className={navDropdownState.file}>
  <button
    onClick={this.toggleDropdownForFile}
    onBlur={this.handleBlur}
    onFocus={this.clearHideTimeout}
    title="File"
    onMouseOver={() => {
      if (this.state.dropdownOpen !== 'none') {
        this.setDropdown('file');
      }
    }}
  >
    <span className="nav__item-header">
      {this.props.t('Nav.File.Title')}
    </span>
    <TriangleIcon
      className="nav__item-header-triangle"
      focusable="false"
      aria-hidden="true"
    />
  </button>
  <ul className="nav__dropdown">

and replace with:

<NavDropdownMenu id="file" title={this.props.t('Nav.File.Title')}>
  • For each menu item, delete a block of code that looks like this:
{this.props.project.id && (
  <li className="nav__dropdown-item">
    <button
      onClick={this.handleShare}
      onFocus={this.handleFocusForFile}
      onBlur={this.handleBlur}
    >
      {this.props.t('Nav.File.Share')}
    </button>
  </li>
)}

and replace with:

<NavMenuItem
  hideIf={!this.props.project.id}
  onClick={this.handleShare}
>
  {this.props.t('Nav.File.Share')}
</NavMenuItem>
  • Move the click-outside and keydown handling into NavBar.
  • Move the dropdownOpen state into NavBar, along with associated logic from toggleDropdown, handleFocus, handleBlur, clearHideTimeout, etc.
  • Delete the this.setDropdown('none'); at the end of each handle function because this is now handled by the NavMenuItem (on the onMouseUp event, so that it doesn't conflict with onClick props).
  • For functions which are now trivial, delete the handler and move the function inline. ie. onClick={this.props.stopSketch} instead of onClick={this.handleStop}.
  • Delete so many function bindings which aren't needed anymore.

The Nav component can continue to be broken up and cleaned up. It's easy to convert it to a function component now because all of the logic has been moved elsewhere. But I wanted to put in this PR while the general structure of Nav is the same so that it's easier to see and review the changes.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for breaking this down and explaining your updates so thoroughly! I think splitting it into those proposed sub-components makes sense and is a great update to the Nav component. I also wasn't too familiar with React Contexts until this PR, and think it's pretty neat!

@raclim raclim merged commit 613bb86 into processing:develop Jun 10, 2023
@lindapaiste lindapaiste deleted the refactor/nav branch August 8, 2023 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor/breakup Nav component
2 participants