Details

Reviewers

tomek
• benschac
atul

Commits

rCOMM2ec3adefc49d: [web] Introduce additional `Menu` styling variants

Summary

Add additional styles for Menu, so it could be used in modal (where the menu design is a bit different)
Default styling (in thread actions menu) remains the same, although the menu will a bit lower than it should. After the next diff (moving rendering into portal) it will look ok.

Menu with thread actions:

Screenshot_Google Chrome_2022-03-09_144006.png (251×186 px, 9 KB)

Menu in modal:

Screenshot_Google Chrome_2022-03-09_143937.png (128×194 px, 7 KB)

Test Plan

Newly added styles are not used yet, but current thread actions menu should remain the same (only its position is not exactly correct)

Diff Detail

Repository

rCOMM Comm

Branch

jacek/menu-variants

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jacek created this revision.Mar 9 2022, 4:12 AM

Herald added subscribers: • benschac, • karol, atul and 2 others. · View Herald TranscriptMar 9 2022, 4:12 AM

Harbormaster completed remote builds in B7235: Diff 10189.Mar 9 2022, 4:17 AM

• jacek requested review of this revision.Mar 9 2022, 4:17 AM

Include some missed styles

Harbormaster completed remote builds in B7237: Diff 10191.Mar 9 2022, 4:26 AM

• jacek edited the summary of this revision. (Show Details)Mar 9 2022, 5:41 AM

• jacek added reviewers: • benschac, atul.

• jacek added a child revision: D3379: [web] Introduce `ThreadMember` component.Mar 9 2022, 5:54 AM

• jacek added a parent revision: D3374: [web] Introduce common `Menu` component.

• benschac requested changes to this revision.Mar 10 2022, 8:07 AM

• benschac added inline comments.

web/components/menu.css
10 ↗	(On Diff #10191)	Why not 1? Just grepped quickly and saw 4 is the highest level of z-index we've used so far in the `web/` project. Also, confused why we need z-index?
22 ↗	(On Diff #10191)	nit: use line-height variable.
41 ↗	(On Diff #10191)	why use top and right if this element isn't absolutely positioned?
69 ↗	(On Diff #10191)	All of the icons with paddings were added to the code base yesterday. Would it make sense to update the size / icon in this diff or a follow up?
web/components/menu.react.js
56–59 ↗	(On Diff #10191)	this could all be one value similar to what we have in where `btnClass`'s second argument is the variant prop passed into the component. https://github.com/CommE2E/comm/blob/master/web/components/button.react.js Rather than explicitly defining each value.

This revision now requires changes to proceed.Mar 10 2022, 8:07 AM

• jacek added a parent revision: D3375: [web] Introduce menu context for displaying actions menu.Mar 14 2022, 11:05 AM

• jacek added inline comments.Mar 14 2022, 11:13 AM

web/components/menu.css
10 ↗	(On Diff #10191)	I see it is not clear in this diff, but in the next one, the menu list will display in a React portal outside the main DOM hierarchy. The aim is to display it over modals and other elements with z-index set.
41 ↗	(On Diff #10191)	`menuActionList` has `position: absolute` (line 14)
69 ↗	(On Diff #10191)	Yes, I'll put u a follow-up diff with the icon change.
web/components/menu.react.js
56–59 ↗	(On Diff #10191)	Ok, seems ok for me. I'll replace current solution with `variant`s

• jacek mentioned this in D3377: [web] use context to display `Menu` action items.Mar 14 2022, 11:42 AM

Used variant prop for menu (like in Button)

• jacek planned changes to this revision.Mar 14 2022, 11:59 AM

Harbormaster completed remote builds in B7355: Diff 10356.Mar 14 2022, 12:02 PM

Revert accidentally pushed changes from the next diff

Harbormaster completed remote builds in B7369: Diff 10375.Mar 15 2022, 4:14 AM

looks good, just have some qustions.

web/components/menu.css
28–29 ↗	(On Diff #10375)	question: why does this need to be positioned with absolute position?
36–37 ↗	(On Diff #10375)	same here: re: absolute position?

This revision is now accepted and ready to land.Mar 15 2022, 5:57 AM

tomek accepted this revision.Mar 15 2022, 10:44 AM

• jacek added inline comments.Mar 18 2022, 5:46 AM

web/components/menu.css
28–29 ↗	(On Diff #10375)	The position of container is measured and set to the top left corner of menu button. So the absolute position is used to display the list below or next to the menu button and not cover it. The thread menu is display below the button, and the members menu is on the left.