fix menu button size, including fixing the additional 2-3px bottom padding from the button element
Confirming vertical-align is the functionality that we want:
Details
Details
check the figma file, additionally, if you want to render the component locally, set the false conditions in thread-menu menu items to true
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- fix-icon-paddings
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/chat/thread-menu.css | ||
|---|---|---|
| 5 ↗ | (On Diff #10044) | https://stackoverflow.com/questions/24626908/how-to-get-rid-of-extra-space-below-svg-in-div-element All of the SWM have display: inline-block embedded into the in-line styling of the icon. I tried verticle-align: top without any luck. This seems like the best solution. |
Comment Actions
All of the SWM have display: inline-block embedded into the in-line styling of the icon.
Can we just change that to avoid having to hack around it?
There's been a lot of churn getting these icons integrated correctly... hopefully we could batch this SVG change with the existing task for integrating all icons (https://linear.app/comm/issue/ENG-822/import-all-icons-from-swm-icon-pack)?
Alternatively, noticed in the SWMansionIcon v2 Dribbble post (https://dribbble.com/shots/17310944-Icons-Figma-Plugin-SWM-Icon-Pack) that there's an accompanying package: https://github.com/kamilagraf/react-swm-icon-pack. Is this something we should consider using instead? The SWM team might have more context on the state of this package, if it's maintained by SWMansion, etc? cc: @palys-swm @def-au1t @karol-bisztyga @adrian @marcinwasowicz
| web/chat/thread-menu.css | ||
|---|---|---|
| 5 ↗ | (On Diff #10044) | I added a style prop to https://phabricator.ashoat.com/D3346 to fix this issue. |
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 188 | We need to use an inline style here: https://github.com/aykutkardas/react-icomoon/blob/master/src/index.tsx#L71 because the Icomoon component uses an inline style. I know this isn't a norm and we typically stay away from inline styles. We're using a block level element so we don't have to hard code a height, https://stackoverflow.com/questions/24626908/how-to-get-rid-of-extra-space-below-svg-in-div-element | |
Comment Actions
ADDRESS INLINE COMMENT BEFORE LANDING
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 188 | We can at least define { display: 'block' } in a higher context so it doesn't get redeclared on every render | |
Comment Actions
I don't knot why having display: inline-block on icons is an issue... it sounds reasonable, and if we want a different behavior, we should wrap it in a tag with different display. Regarding the icon pack, I would need to ask some people. This project on github isn't hosted on official Software Mansion account - not sure what's the reason for that.
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 175 ↗ | (On Diff #10162) | Why didn't you just move this to the top level (outside of the ThreadMenu function)? This works too but the React.useMemo seems a bit unnecessary |
Comment Actions
I don't knot why having display: inline-block on icons is an issue... it sounds reasonable, and if we want a different behavior, we should wrap it in a tag with different display. Regarding the icon pack, I would need to ask some people. This project on github isn't hosted on official Software Mansion account - not sure what's the reason for that.
instead of changing the display of inline block I just changed the behavior of SVG in button elements to align correctly per https://stackoverflow.com/questions/24626908/how-to-get-rid-of-extra-space-below-svg-in-div-element.
I think this solution makes the most sense.