[web] Added extra margin in "Create subchannel" modal
ClosedPublic
Actions

Authored by • przemek on Oct 7 2022, 2:34 AM.

Details

Reviewers

tomek
kamil
atul
rohan
ginsu

Commits

rCOMM13e9f0807ca1: [web] Added extra margin in "Create subchannel" modal

Summary

Changed margin in wrapper around Visibility Option Content in Create
Subchannel modal.

Screenshot 2022-10-07 at 11.50.50.png (1×854 px, 117 KB)

Test Plan

Build app.
Visually checked if it looks correctly after changes.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• przemek created this revision.Oct 7 2022, 2:34 AM

Herald added subscribers: • abosh, ashoat. · View Herald TranscriptOct 7 2022, 2:34 AM

Harbormaster completed remote builds in B12671: Diff 17407.Oct 7 2022, 2:52 AM

• przemek requested review of this revision.Oct 7 2022, 2:52 AM

• przemek edited the summary of this revision. (Show Details)Oct 7 2022, 2:52 AM

• przemek edited the test plan for this revision. (Show Details)

• przemek added 1 blocking reviewer(s): tomek.

• przemek retitled this revision from Added margin in create channel to [web] Added margin in create channel.

• przemek retitled this revision from [web] Added margin in create channel to [web] Added extra margin in "Create subchannel" modal.Oct 7 2022, 4:14 AM

Hey @przemek, your attachment didn't work because of a quirk in Phabricator, unfortuantely... read more here

• przemek attached a referenced file: F193501: Screenshot 2022-10-07 at 11.50.50.png. (Show Details)Oct 7 2022, 7:11 AM

Hi @ashoat, thanks for pointing this out. It should be fixed now

atul added reviewers: rohan, ginsu.Oct 7 2022, 7:47 AM

Looks good!

Some tips for next diffs you push up:

In the description linking the linear task would be helpful so that reviewers can gain more context of what you are working on
For UI changes, it is nice to include two screenshots (a before and after of the changes you made) so that reviewers can easily see the change

atul accepted this revision.Oct 7 2022, 11:06 AM

atul added inline comments.

web/components/enum-settings-option.css
34 ↗	(On Diff #17407)	I think it might sense to use `padding` (internal to `optionContent`) instead of `margin` (external to `optionContent`) here... defer to @tomek who is blocking reviewer.

tomek accepted this revision.Oct 10 2022, 2:17 AM

tomek added inline comments.

web/components/enum-settings-option.css
34 ↗	(On Diff #17407)	I think that `margin` makes sense - it is a space between elements and not something internal to the element. If we had a background color here, I think that it shouldn't touch the input icon (but at the same time, some padding would also be added). Usually a better way of doing this is for the container to decide how to position its children. So e.g. use `gap`, `justify-content`, etc. But margin is good enough for this simple use case.