fix: Default `Menu` to have modal mode enabled

frankieyan commented 1 year ago

Addresses https://github.com/Doist/Issues/issues/9053

Short description

Previously, modal was false on Ariakit's MenuList, so that interaction with elements underneath the menu while it was open was possible. This isn't a behaviour we had in our legacy menus, and it also causes bugs such as unwanted tooltips appearing (especially on the trigger for the currently open menu). These interactions will now be blocked by default.

PR Checklist

[ ] Added tests for bugs / new features
[x] Updated docs (storybooks, readme)
[x] Executed npm run validate and made sure no errors / warnings were shown
[ ] Described changes in CHANGELOG.md
[ ] Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
[ ] Updated all static build artifacts (npm run build-all)

Versioning

Breaking

gnapse commented 1 year ago

Why do you consider this a breaking change, @frankieyan? Does it involve a change in the public-facing API? Or do you consider it breaking because of the new menu behavior? If it's the latter, I would not consider that a breaking change. For me, a breaking change is only when the public-facing API of the library changes in ways that can break the apps that use the library.

rfgamaral commented 1 year ago

For me, a breaking change is only when the public-facing API of the library changes in ways that can break the apps that use the library.

I fully agree with this, but I guess the reasoning was that this "breaks known behaviour". Everything is still functional, but if one has e2e tests depending on the previous behaviour, this change will break them. But then again, that's not public-facing either. With that said, I vote for this to be a patch, at most a minor.

gnapse commented 1 year ago

if one has e2e tests depending on the previous behaviour

That's a good point. However, taken to the extreme, it could mean that virtually any change is a breaking change, because virtually any previous behavior could be encoded in a test, even bugs could've been tested for. Although in this case I agree it can be more natural to fall into, since any test interacting with menus might need to change, right? Anyway, I leave it to Frankie to decide after considering our comments.

I vote for this to be a patch, at most a minor.

Either a patch (fix) or a major (breaking). Never a minor. A minor is a new feature or capability that the consumers may or may not take advantage of. Makes sense?

gnapse commented 1 year ago

BTW, @frankieyan, I can wait to merge my new Badge release (#744), which is a breaking change, in case you want to make this a breaking change we can combine them both into a single release. I already prepared the release in that PR, so it should be a matter of merging this one first, then rebasing mine on top and updating the static assets before merging it.

rfgamaral commented 1 year ago

A minor is a new feature or capability that the consumers may or may not take advantage of. Makes sense?

You're right, yes, it makes sense 👍

frankieyan commented 1 year ago

@gnapse I was planning on releasing this with https://github.com/Doist/reactist/pull/734, which is also a breaking change. That one involves a bit more testing in TD/TW, so if you wanted the badge change to go out first please go ahead, and I'll cut a subsequent release after 👍

I was also split between a major or patch designation for this one. Potentially broken tests (there was one for the menu's own tests) and this being a potentially unwanted behaviour for some cases, plus having another breaking change coming made me go with a breaking change. It's a moot point if we release the two changes together either way.

gnapse commented 1 year ago

It's a moot point if we release the two changes together either way.

Yup, that's what I was thinking. Just put it alongside another breaking change and that's it.

I'm not super hurried with the badge (not yet working on what needs them in TD). But probably by Friday or Monday I will release them on their own if you don't ping me earlier to put all three changes in one release.

frankieyan commented 1 year ago

@gnapse Ok, I'll get these two out and into both apps first 👍

gnapse commented 1 year ago

I'll get these two out and into both apps first 👍

Wait, what I meant was: if you're ready to release both of your PRs before I absolutely have to merge mine with the badges, then we can bundle the 3 PRs in one release. Mine is ready to be merged when you are.

Doist / reactist