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.

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

fix: filter menu button collapsing goal state #511

Merged
merged 4 commits into from
Aug 4, 2024

Conversation

mhuisi
Copy link
Collaborator

@mhuisi mhuisi commented Jul 26, 2024

Closes #505.

@mhuisi mhuisi requested a review from Vtec234 July 26, 2024 11:07
@Vtec234
Copy link
Member

Vtec234 commented Jul 30, 2024

As far as I can tell, the underlying issue is not really about native vs synthetic React events (BTW, do you have a source for docs about this?). It is more simply that there were a couple of places where we call stopPropagation but not preventDefault. This means that handlers higher up do not get called, but the default event behaviours still occur. You can see it in this example app:

export default function App() {
  return (
    <details>
      <summary onClick={e => e.preventDefault()}>
        <a onClick={e => e.stopPropagation()}>EFFECT</a>
        <span> - </span>
        <a>NO EFFECT</a>
      </summary>
      Content
    </details>
  )
}

So, I propose an alternative fix where we make sure that whenever stopPropagation is called, so is preventDefault. I made this change systematically, and it seems to work (but more testing would be good). I also changed the semantics of Details a bit to inject the <summary> because we should really install the onClick handler there rather than on <details>. I pushed the changes here, but feel free to revert if something is not working.

@mhuisi
Copy link
Collaborator Author

mhuisi commented Jul 30, 2024

I'm okay with you taking over this PR. Please proceed as you see fit. Your changes look good to me but I haven't tested them in detail.

BTW, do you have a source for docs about this?

https://stackoverflow.com/questions/52444941/when-does-react-create-syntheticevents (and several others, though I can't find documentation of it in the React docs).

I agree that this is due to a lack of preventDefault and not a native browser event firing - I had a misconception about the way the <details> toggle works here and how preventDefault and stopPropagation interact. I think my version worked because it uses a native browser event to call preventDefault and is hence not stopped by the stopPropagation calls in the synthetic events that we use everywhere else.

@Vtec234 Vtec234 merged commit 93095f9 into master Aug 4, 2024
2 checks passed
@Vtec234 Vtec234 deleted the mhuisi/infoview-filter-button-issue branch August 4, 2024 15:55
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.

Tactic state collapses when filters are opened
2 participants