You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I think this must need a rebase, and I suspect it will be of a bigger magnitude than I should be touching? Along the way, Claude suspects one "dormant" bug and has some nits. Coming next.
The PTXDropdown approach is the right move — proper menu semantics and keyboard handling, and it fixes the invalid <div>-inside-<button> nesting the old profile menu had. A few things.
One actual bug:
In the PTXDropdown constructor, the "close on escape" listener dereferences this.controlElement outside the if (this.controlElement) guard that wraps the other control-element listeners:
// Close on escape even if opened via click and focus is on buttonthis.controlElement.addEventListener("keydown",(event)=>{if(event.key==="Escape"&&this.isOpen()){
Since openButton defaults to null and you use this.controlElement?.contains(...) elsewhere, the class is meant to tolerate a null control — so new PTXDropdown(menuEl) would throw here. The current call site passes a button, so it doesn't bite today. Folding this listener into the existing if (this.controlElement) block fixes the crash and also drops a duplicate Escape path (it's already handled in handleKeydown). That inner if is also mis-indented.
Smaller:
console.log("PTXDropdown: No dropdown element provided.") reads more like a console.warn.
pretext-dropdown.js has no trailing newline.
XSL: aria-labelledby="ptx-user-dropdown-button" role="menu" has a double space; and aria-label="Runestone" is the one hardcoded string in the new menu (probably fine as a product name).
Worth a visual check: the old ptx-dropdown-button mixin set position: relative on the button, and the renamed ptx-dropdown-menu mixin (now on the menu element) uses position: absolute; top: 100%; right: 0 without that anchor — confirm the menu still positions under the button rather than against a higher navbar ancestor.
Claude Opus 4.8, acting as a review assistant for Rob Beezer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds PTXDropdown framework and uses that to make Profile menu accessible.
Gets rid of hard coded RS menu items.
This builds on #2952
Starting at f46495a
After that PR is in I can rebase to that point.