Details

web/chat/edit-text-message.react.js
21 ↗	(On Diff #26489)	There will be two copies of this component. One in the background, which will be used to determine the position of the second one - modal. This property will be used to differentiate between those.

web/chat/edit-text-message.react.js
30 ↗	(On Diff #26489)	It seems like this component would be better as a function component
79 ↗	(On Diff #26489)	Can we just pass `this.props.clearEditModal` in directly?
120–121 ↗	(On Diff #26489)	Let's avoid mixing default and non-default exports

michal requested changes to this revision.May 16 2023, 11:01 AM

michal added inline comments.

web/chat/edit-text-message.react.js
21 ↗	(On Diff #26489)	Will the background component be used for anything other than the position? Will it be interactable?
79–82 ↗	(On Diff #26489)	Can we use our `Button` component?

This revision now requires changes to proceed.May 16 2023, 11:01 AM

Changed to the function component, addressed review

web/chat/edit-text-message.react.js
21 ↗	(On Diff #26489)	No, it is used only to calculate and update the position of the foreground element. It won't be interactable.
79–82 ↗	(On Diff #26489)	Great catch! I didn't notice it. Changed.

michal requested changes to this revision.May 17 2023, 5:59 AM

michal added inline comments.

web/chat/edit-text-message.css
48–57 ↗	(On Diff #26575)	I think some of these are now unnecessary if you are using `<Button>`. In particular, the rest of our buttons have a border-radius of 8px. If designs don't call for anything else we should keep it the same.
web/chat/edit-text-message.react.js
74–84 ↗	(On Diff #26575)	Is there a reason why we split it into multiple components? Normally we do it when we have an older class component and we want to use hooks.

This revision now requires changes to proceed.May 17 2023, 5:59 AM

• kuba planned changes to this revision.May 17 2023, 6:30 AM

• kuba marked an inline comment as done.

Addressed review

web/chat/edit-text-message.css
48–57 ↗	(On Diff #26575)	Right, removed most of them. `border-radius` in designs has 4px, probably because it is a smaller button.
web/chat/edit-text-message.react.js
74–84 ↗	(On Diff #26575)	Right! Forgot to remove after switching to function component.

Just a few more points about the buttons, otherwise looks good!

web/chat/edit-text-message.react.js
32 ↗	(On Diff #26620)	I think `var(--fg)` is the default so you can remove it.
61–66 ↗	(On Diff #26620)	Not sure how troublesome it would be but `Button` has `filled` and `text` variants that could be used here. It would give us a nice animation on the "Save" button. But if it turns out to be more complicated, we can keep this. You should be able to change the border-radius for the `filled` variant by setting the `--border-radius` css variable in the style prop.

This revision now requires changes to proceed.May 17 2023, 7:09 AM

web/chat/edit-text-message.react.js
30–33 ↗	(On Diff #26620)	You're redefining this on every invocation right now, which is very bad for performance. Please wrap in a `useMemo`
34–36 ↗	(On Diff #26620)	If you end up keeping this, it should be defined at the component root. You're redefining it on every invocation right now, which is very bad for performance

Address review

web/chat/edit-text-message.react.js
25 ↗	(On Diff #26622)	I added it to remove the default `filled` variant background. I had to set the `filled`variant because otherwise it wasn't vertically centered correctly.

Rebase

web/chat/edit-text-message.react.js
25 ↗	(On Diff #26639)	Should this just be `transparent`? Can this be moved into `edit-text-message.css`?
34–38 ↗	(On Diff #26639)	Shorthand

web/chat/edit-text-message.css
3 ↗	(On Diff #26639)	Probably should be `var(--modal-bg)`?
18–25 ↗	(On Diff #26639)	Nit: could this be merged and applied directly to the icon?
web/chat/edit-text-message.react.js
25 ↗	(On Diff #26639)	Would `background: transparent` in the .css file work instead of this?

Address review

web/chat/edit-text-message.css
18–25 ↗	(On Diff #26639)	Did you mean simply moving it to the `.iconContainer`? If so, it doesn't work.
web/chat/edit-text-message.react.js
25 ↗	(On Diff #26639)	I don't think it can be easily moved to the `edit-text-message.css` without touching the `Button` component. `Button` component completely ignores extra styles (lines 54-61 of `button.react.js`, we don't merge styles with style prop), it always overrides the background color when we set variants to either `outline` or `filled`, I have to set the variant to one of these (`outline`/`filled`). Otherwise, it is not correctly positioned (it doesn't use the `.btn` class in line 48 of `button.react.js`).

Removed width property

michal added inline comments.

web/chat/edit-text-message.css
18–25 ↗	(On Diff #26639)	I mean merging it into .icon { color: var(--error); height: 16px; display: flex; } applying it to the `XCircleIcon` and removing the icon container.