Skip to content
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.

Sign up for GitHub

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

Masterbar: fix invalid HTML markup #98510

Merged
merged 5 commits into from
Jan 21, 2025
Merged

Masterbar: fix invalid HTML markup #98510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
433cccb
Reorganize and simplify markup:
ciampo Jan 16, 2025
dadcbd6
Add back subitems, tweak CSS selectors to match new markup
ciampo Jan 16, 2025
27215d8
Remove code comments
ciampo Jan 19, 2025
7c01260
Revert to && instead of ternary
ciampo Jan 19, 2025
abd76a2
Keep submenu opened while hovering
ciampo Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 16 additions & 40 deletions client/layout/masterbar/item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ class MasterbarItem extends Component< MasterbarItemProps > {
isOpenForNonMouseFlow: false,
};

componentButtonRef = React.createRef< HTMLButtonElement >();
componentDivRef = React.createRef< HTMLDivElement >();
Comment on lines -63 to -64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By always rendering the wrapper, and either and a or a button, componentButtonRef is not necessary anymore, and componentDivRef was renamed to wapperRef for better clarity

wrapperRef = React.createRef< HTMLDivElement >();

_preloaded = false;

Expand Down Expand Up @@ -191,12 +190,8 @@ class MasterbarItem extends Component< MasterbarItemProps > {
}

// Check refs to see if the touch event started inside our component, if it didn't, close the menu.
const isInComponentButtonRef = this.componentButtonRef.current?.contains(
event.target as Node
);
const isInComponentDivRef = this.componentDivRef.current?.contains( event.target as Node );

if ( ! isInComponentButtonRef && ! isInComponentDivRef ) {
Comment on lines -194 to -199
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to the comment above, since the wrapper button was removed, the logic can be simplified to only check whether we're in the wrapper div (which is now always rendered)

const isInWrapper = this.wrapperRef.current?.contains( event.target as Node );
if ( ! isInWrapper ) {
this.setState( { isOpenForNonMouseFlow: false } );
}
};
Expand All @@ -221,49 +216,30 @@ class MasterbarItem extends Component< MasterbarItemProps > {
disabled: this.props.disabled,
};

if ( this.props.url && ! this.props.subItems ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from

if ( this.props.url && ! this.props.subItems ) {
  // render anchor tag
}

if ( this.props.url && this.props.subItems ) {
  // render button with anchor tag and sub-menu items as children
}

// else, if props.url is not defined
// render a wrapper div > button > menu items as children (no anchor tag)

to:

  • always rendering a wrapper div
  • render either a button or an a depending on props.url
  • render the sub-menu items (when defined) as sibling of the button/a

return (
const MenuItem = ( props: React.HTMLAttributes< HTMLElement > ) =>
Copy link
Member

Choose a reason for hiding this comment

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

FYI it's not a good idea to create components inside a render method. It causes the component to be recreated on every render which can have side effects (like this). See #98897 which should fix this issue by moving the component's definition outside the render method.

this.props.url ? (
<a
{ ...attributes }
href={ this.props.url }
ref={ this.props.innerRef as LegacyRef< HTMLAnchorElement > }
>
{ this.renderChildren() }
</a>
{ ...props }
/>
) : (
<button ref={ this.props.innerRef as LegacyRef< HTMLButtonElement > } { ...props } />
);
}

if ( this.props.url && this.props.subItems ) {
return (
<button
{ ...attributes }
ref={ this.componentButtonRef }
onKeyDown={ this.toggleMenuByKey }
>
<a
href={ this.props.url }
ref={ this.props.innerRef as LegacyRef< HTMLAnchorElement > }
onTouchEnd={ this.toggleMenuByTouch }
tabIndex={ -1 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we don't render a/button nested in a parent button, the tabindex={-1} shouldn't be necessary anymore

>
{ this.renderChildren() }
</a>
{ this.renderSubItems() }
</button>
);
}

return (
<div className={ this.props.wrapperClassName } ref={ this.componentDivRef }>
<button
<div
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always rendering the div could cause some styles to break, but I hopefully managed to catch and amend them all. Lots of testing needed to make sure I didn't miss any edge case

className={ clsx( 'masterbar__item-wrapper', this.props.wrapperClassName ) }
ref={ this.wrapperRef }
>
<MenuItem
{ ...attributes }
ref={ this.props.innerRef as LegacyRef< HTMLButtonElement > }
onKeyDown={ this.props.subItems && this.toggleMenuByKey }
onTouchEnd={ this.props.subItems && this.toggleMenuByTouch }
>
{ this.renderChildren() }
{ this.renderSubItems() }
</button>
</MenuItem>
{ this.renderSubItems() }
</div>
);
}
Expand Down
Loading
Loading