chore(ui): Clean up settings component for shared uses #34090
Conversation
agg23
changed the title
This comment has been minimized.
This comment has been minimized.
|
Does it change UI? If it does, please attach before / after screenshots! |
| }; | ||
| return ( | ||
| <div className='vbox settings-view'> | ||
| {settings.map(({ value, set, name, title }) => { |
There was a problem hiding this comment.
If setting becomes a class, destructuring will cause problems (illegal this in set invocation)
| @@ -15,25 +15,27 @@ | |||
| */ | |||
|
|
|||
| .settings-view { | |||
| display: flex; | |||
There was a problem hiding this comment.
nit: you are using it in combination with vbox, you are flex already. we would usually either use vbox or define flex style, but not both.
| type='checkbox' | ||
| id={labelId} | ||
| checked={value} | ||
| onChange={() => set(!value)} |
There was a problem hiding this comment.
That'll probably cause re-render - you know better though!
There was a problem hiding this comment.
It does. There's a lot of rerender perf issues present right now across the UI. That will be a future PR if we decide to fix it.
| } | ||
|
|
||
| .settings-view .setting input { | ||
| margin-right: 5px; | ||
| } | ||
| flex-shrink: 0; | ||
| } |
There was a problem hiding this comment.
we like new lines in the end of file :P
Test results for "tests 1"9 flaky37388 passed, 650 skipped Merge workflow run. |
Separated out changes for settings from #34010 and #34058.