Doist / reactist

Open source React components made with ❤️ by Doist
http://doist.github.io/reactist
MIT License
247 stars 21 forks source link

feat: New menu styles #792

Closed gnapse closed 1 year ago

gnapse commented 1 year ago

Short description

This brings in from Todoist the CSS styles we use for menus in Todoist and Twist.

Up until now, we've had the menus in Reactist styles in more basic ways, and dealt with the style changes as CSS overrides in the app code. This PR now makes Reactist provide the menu styling, as Reactist does for all its other components.

New sub-menu components

Additionally, this introduces two new menu components: SubMenuItem and SubMenuList. This was necessary for a few reasons:

  1. These had specific styling needs that could not be addressed in the way we used to do sub-menus. The app was dealing with this in an ad-hoc way, but to achieve the library providing it, we needed the dedicated components.
  2. In the specific case of SubMenuItem, we needed it because we needed a component that acted as a sub-menu button, but with an interface and styling closer to that of the MenuItem.
  3. In the specific case of the SubMenuList, it needs a tiny but important dedicated CSS rule to have it be aligned with the sub menu item that triggered the sub-menu (see screenshot below, including the added orange lines to show what I'm alignment this is about): CleanShot 2023-07-07 at 14 30 57@2x

Test plan (IMPORTANT)

Even though this is not going to be released after merging (see note in the "Versioning" section below) it is very important that you help me validate that this is good to go to use in the apps.

I'll assign as reviewer in Todoist the same reviewer as this PR, so that you can execute the test plan I'll put over there.

PR Checklist

Versioning

Note This PR's base branch is not main. So approving this does not yet mean it will be released right away. I'm planning a series of improvements to the menu component, that I'll gather in this base branch before I make a single release (or a handful of releases). This will allow me to test the changes with Todoist before comitting Reactist to the new features.

jvalente commented 1 year ago

The menu positioning within limited screen space seems to need some tuning. Some instances exist where the menu renders up or down with scrolling while enough vertical space exists to render the full menu:

252352473-e5308aa0-8330-4d87-9c6b-6a986026f0bc

Let me know if we should create a new issue about it. 👍

--

Code looks great. 🙌

gnapse commented 1 year ago

The menu positioning within limited screen space seems to need some tuning. Some instances exist where the menu renders up or down with scrolling while enough vertical space exists to render the full menu:

Could you confirm if this is something new when using this branch of Reactist? Or can you reproduce in Todoist without this using this branch?

jvalente commented 1 year ago

Could you confirm if this is something new when using this branch of Reactist?

The behavior introduced here is new and better since, as it is, menu actions become inaccessible under limited screen space.

image