Page MenuHomePhabricator
Differential D5115

[web] Added possibility to change input type and icon position in EnumSettingsOption
ClosedPublic
Actions

Authored by • jakub on Sep 12 2022, 8:52 AM.
Tags
None
Referenced Files
F3350111: D5115.id16812.diff
Fri, Nov 22, 8:48 PM
Unknown Object (File)
Thu, Nov 21, 9:44 AM
Unknown Object (File)
Thu, Nov 7, 9:19 PM
Unknown Object (File)
Thu, Nov 7, 9:19 PM
Unknown Object (File)
Thu, Nov 7, 9:19 PM
Unknown Object (File)
Thu, Nov 7, 9:19 PM
Unknown Object (File)
Thu, Nov 7, 9:19 PM
Unknown Object (File)
Thu, Nov 7, 9:19 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
tomek
atul
ashoat
Commits
rCOMM8e2208623fcd: [web] Added possibility to change input type and icon position in…
Summary

Depends on D5110
Context: here

Added possibility to change input type of EnumSettingsOption from "radio" to "checkbox" and added iconPosition param to set vertical position of the left icon.

Additionally, setting cursor icon to pointer, when we hover on it.

Now, according to Figma design, we could use one component to create both radio and checkbox options.

image.png (161×382 px, 10 KB)

Test Plan

Currently, this improvement hasn't been used yet. It will be used in next diff.

To change EnumSettingsOption input type to checkbox, set inputType param as a "checkbox".

To change a vertical position of the left icon, set iconPosition as one of the {"top", "center", "bottom"} (by default is set to "center")

Diff Detail

Repository
rCOMM Comm
Branch
jakub/channel-composer
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jakub created this revision.Sep 12 2022, 8:52 AM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptSep 12 2022, 8:52 AM
Harbormaster completed remote builds in B12089: Diff 16590.Sep 12 2022, 9:02 AM
• jakub requested review of this revision.Sep 12 2022, 9:02 AM
• jakub edited the summary of this revision. (Show Details)Sep 12 2022, 9:25 AM
• jakub edited the test plan for this revision. (Show Details)
• jakub added reviewers: tomek, atul, ashoat.
• jakub added a parent revision: D5113: [web] Introducing `ModalLabel` component.
atul accepted this revision.Sep 15 2022, 8:32 AM

Looks good, left some comments to be addressed before landing

web/components/checkbox.css
7

Can we use px units here? IIRC the only time we use % units for border-radius is when it's 50%.

17

Same feedback as above regarding % and px units.

web/components/enum-settings-option.react.js
35–36

Should we set a default value for type here like we are for iconPosition?

58

Can we change this to

type === 'checkbox'

for consistency with the rest of our codebase?

(WebStorm's default style rules would suggest putting 'checkbox' before type here.. might be worth turning that rule off if you happen to be using WebStorm)

74–81

We can probably define this outside of the component altogether?

This revision is now accepted and ready to land.Sep 15 2022, 8:32 AM
• jakub updated this revision to Diff 16741.Sep 16 2022, 1:54 AM
• jakub edited the summary of this revision. (Show Details)

Changing % to px units, set default value of type param, move iconPositionClassNames outside of the component and adjust to codebase

Harbormaster completed remote builds in B12206: Diff 16741.Sep 16 2022, 2:13 AM
ashoat resigned from this revision.Sep 16 2022, 7:38 AM

I should not be a first-pass reviewer except for the cases listed here

tomek requested changes to this revision.Sep 16 2022, 11:06 AM
tomek added inline comments.
web/components/checkbox.css
23 ↗(On Diff #16741)

It's great that we're using pseudo-class here - it's a lot more convenient when debugging

web/components/checkbox.react.js
10 ↗(On Diff #16741)

We already have a different kind of checkbox in the codebase (used on calendar tab -> filters). It looks differently, so probably can't be reused, but definitely it's worth to take a look.

17 ↗(On Diff #16741)

We should definitely have this as a prop instead of hardcoding it

web/components/enum-settings-option.react.js
26–32 ↗(On Diff #16741)

Can we modify this code so that keys in iconPositionClassnames are consistent with IconPosition type? Maybe we can use $Keys?

This revision now requires changes to proceed.Sep 16 2022, 11:06 AM
• jakub updated this revision to Diff 16812.Sep 19 2022, 2:06 AM

Set readOnly as optional param and use $Keys instead of hardcoding IconPosition type

Harbormaster completed remote builds in B12251: Diff 16812.Sep 19 2022, 2:16 AM
• jakub added a child revision: D5185: [web] Make `AddMembersList` more universal.Sep 20 2022, 2:12 AM
tomek accepted this revision.Sep 20 2022, 10:21 AM
tomek added inline comments.
web/components/enum-settings-option.react.js
25–32 ↗(On Diff #16812)

Maybe we should move it before Props where it is used?

This revision is now accepted and ready to land.Sep 20 2022, 10:21 AM
• jakub updated this revision to Diff 16923.Sep 20 2022, 11:28 AM

Rebase and move additional types declarations before Props type

Harbormaster completed remote builds in B12330: Diff 16923.Sep 20 2022, 11:39 AM
• jakub edited the summary of this revision. (Show Details)Sep 26 2022, 3:30 AM
• jakub added a parent revision: D5110: [web] Fix warning - set readOnly param to EnumSettingOption radio.
• jakub updated this revision to Diff 17073.Sep 26 2022, 3:37 AM

Rebase before landing

Harbormaster completed remote builds in B12432: Diff 17073.Sep 26 2022, 3:48 AM
Closed by commit rCOMM8e2208623fcd: [web] Added possibility to change input type and icon position in… (authored by • jakub). · Explain WhySep 26 2022, 3:55 AM
This revision was automatically updated to reflect the committed changes.
• jakub added a commit: rCOMM8e2208623fcd: [web] Added possibility to change input type and icon position in….
ashoat added inline comments.Sep 26 2022, 4:52 AM
web/components/checkbox.react.js
1 ↗(On Diff #17074)

Nit: please try to maintain style consistent with the rest of the codebase. Note that we always include an extra newline following the // @flow declaration

Revision Contents
Changeset List

PathSize
web/
components/
25 lines
22 lines
13 lines
45 lines

Diff 16590

View Options

web/components/checkbox.css

Loading...
View Options

web/components/checkbox.react.js

Loading...
View Options

web/components/enum-settings-option.css

Loading...
View Options

web/components/enum-settings-option.react.js

Loading...