Closed natalynyu closed 3 years ago
Merging #75 into develop will increase coverage by
0.10%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## develop #75 +/- ##
===========================================
+ Coverage 98.12% 98.23% +0.10%
===========================================
Files 41 42 +1
Lines 480 509 +29
Branches 87 93 +6
===========================================
+ Hits 471 500 +29
Misses 5 5
Partials 4 4
Impacted Files | Coverage Δ | |
---|---|---|
src/Forms/CheckLabel.tsx | 100.00% <100.00%> (ø) |
|
src/Forms/Checkbox.tsx | 100.00% <100.00%> (ø) |
|
src/Forms/Dropdown.tsx | 100.00% <100.00%> (ø) |
|
src/Forms/Fieldset.tsx | 100.00% <100.00%> (ø) |
|
src/Forms/InputLabel.tsx | 96.77% <100.00%> (ø) |
|
src/Forms/RadioButton.tsx | 96.00% <100.00%> (+4.33%) |
:arrow_up: |
src/Forms/TextInput.tsx | 100.00% <100.00%> (ø) |
|
src/Forms/index.ts | 100.00% <100.00%> (ø) |
|
... and 1 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 4d8867b...1f1c998. Read the comment docs.
@jonseitz Thanks for your feedback! Those are good points. I was wondering: in the new implementation of the Label
component, should the input fields still line up (and if so, would the text of the longer descriptions below wrap to the next line)?
@natalynyu Yes, the current behavior is to wrap the line:
Just to clarify though, my suggestion for a new implementation of the Label
was that we'd be creating one that would only be used for checkboxes/radio buttons, so I wasn't thinking that the new Label
would affect the text inputs at all. Does that make sense?
@jonseitz Ah right! I confused myself - sorry about that. I understand now - so we'd be keeping the original behavior of the label in relation to the Text Input
component.
@jonseitz For the example screenshot that you posted, where would the radio/checkboxes be located in relation to the group of text inputs?
Would the radio and checkbox labels in these cases line up with the text input field? That seems more normal looking to me, but for the case of the course admin modal, I would think that the checkboxes are aligned on the very left where the green line I drew in is above.
For the case of the course admin modal, though, the text label position is POSITION.TOP
as opposed to POSITION.LEFT
in the example above. I think for the modal we would want each line to be left-aligned:
Hmmmm, very good questions. Thinking just about the checkboxes/radios, my sense is:
labelPosition={POSITION.RIGHT}
, the checkbox and label should be adjacent and flush all the way leftlabelPosition={POSITION.LEFT}
the grid should be the same as the text inputs, with label in line with the other labels and the checkbox in line with the inputslabelPosition={POSITION.TOP}
both should be flush leftSo maybe it's really just the case of POSITION.RIGHT
where we need to make changes in the Label
?
@jonseitz I tried mapping out each of the cases, and based on the criteria you outlined, it looks we would also need to make changes when the
POSITION.TOP
and the checkbox/radio button label is POSITION.LEFT
: The other two cases to solve for would be when the
POSITION.TOP
and the checkbox/radio position is POSITION.RIGHT
:POSITION.LEFT
and the checkbox/radio position is POSITION.RIGHT
:So I could make a new label component that solves for these cases?
I think we should handle the check/radio and input/select separately -- It's just going to be too complicated to try to make every possible combination of labels/inputs line up correctly.
So really, I think we just need to change the POSITION.RIGHT
option for the check/radio so that it's flush left, and then we should add a note to our docs that we should use that versions whenever we're using it in conjunction with a select/input that uses POSITION.TOP
. What do you think about that?
@jonseitz Ok, yes I think accounting for all situations at once may be too complicated. I'm not really sure what you meant by your suggestion. Do you mean making a second Label
component that is just used for the case when the text input has a labelPosition of POSITION.TOP
& the check/radio's label is POSITION.RIGHT
/POSITION.LEFT
?
Right now, we have four components that are all using the same Label
TextInput
Dropdown
Checkbox
RadioButton
I'm proposing that we:
TextInput
and Dropdown
componentsLabel
(e.g. InputLabel
), allowing POSITION.TOP
and POSITION.LEFT
, and only use that in TextInput
and Dropdown
CheckLabel
, allowing POSITION.LEFT
and POSITION.RIGHT
, and use that in place of Label
in the the Checkbox
and RadioButton
components.In the InputLabel
: POSITION.TOP
and POSITION.LEFT
would stay exactly as they are
In the CheckLabel
: we'd switch to using a flush-left style, like what you'd originally done here, with POSITION.RIGHT
and leave POSITION.LEFT
as it is.
That way, if you're using a TextInput
with POSITION.TOP
, you can use a Checkbox
with POSITION.RIGHT
beneath it, and both input "boxes" will be flush-left.
Then if you have a TextInput
with POSITION.LEFT
, you can use a Checkbox
with POSITION.LEFT
beneath it, and the labels and inputs will line up.
Technically, this doesn't actually require us to create separate label components. However, I feel like we've already added a lot of conditional logic to make that a single Label
adapt to all the different possible POSITION
values with the two types of inputs, and I'm concerned about longterm maintainability as we add more components down the line. Ultimately we're not using the Label
elements directly when we import mark-one
into the app, so this shouldn't affect the public API of the library at all.
@jonseitz thanks for your help on this! I made the CheckLabel
component and renamed the original Label
to InputLabel.
I also added docs a few more tests, and I exported the POSITION
enum from InputLabel
so that it can be used in the docs.
The purpose of this PR is to fix the formatting for the
RadioButton
andCheckbox
components so that when they are used in a form in conjunction with theTextInput
component, they are all appropriately aligned at the same start point. With the previous state of these components, this is how the components look when used together:I added![image](https://user-images.githubusercontent.com/29589689/95911775-e639a300-0d6f-11eb-9a57-a3bd26724b0f.png)
TextInput
andRadioButton
components to theModalBody
overflow example to demonstrate the alignment of the components with the updated code:Types of changes
Checklist:
eslint
on the codePriority:
Related Issues:
Addresses #229