-
Notifications
You must be signed in to change notification settings - Fork 486
Conversation
<clr-checkbox-wrapper | ||
*ngFor="let opt of fieldChoices(field); trackBy: trackByFn" | ||
> | ||
<cds-form-group control-width="shrink"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think layout is working as expected and only allowed for radio buttons/checkboxes. It doesn't solve the original issue though
66e9795
to
1465387
Compare
pkg/view/component/form.go
Outdated
@@ -117,6 +118,126 @@ func marshalFormField(ff FormField) ([]byte, error) { | |||
return json.Marshal(&m) | |||
} | |||
|
|||
type FormFieldLayout struct { | |||
*BaseFormField | |||
value string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be something more descriptive like row size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the values of a form, I don't think I am adding anything extra to this field
pkg/view/component/form.go
Outdated
@@ -117,6 +118,126 @@ func marshalFormField(ff FormField) ([]byte, error) { | |||
return json.Marshal(&m) | |||
} | |||
|
|||
type FormFieldLayout struct { | |||
*BaseFormField |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a form field type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, I have three solutions for this
- Add a boolean field to Base Form Field to see if it should be horizontal or not but this idea has a flaw
- Add a int to know the amount of fields going into a row
- Create a struct to save that information there
I think none of them are great solutions, but that's why I wanted to talk before implementing this
pkg/view/component/form.go
Outdated
} | ||
|
||
func (ff *FormFieldLayout) SetLabel(value string) { | ||
ff.label = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can intuitively see the problem of making a field layout into a form field - labels and values are forced to be set even though there are largely not relevant in this context.
This is more apparent under UnmarshalJSON with error, name, validators, etc.
pkg/view/component/form.go
Outdated
return ff.value | ||
} | ||
|
||
func (ff *FormFieldLayout) UnmarshalJSON(data []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo, this is a lot of extra churn just to insert a class:
I think the preferred way to fix this is to implement the to-do (convert FormField into a struct).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is not the class but rather the div that provides the horizontal layout
d1fe1d1
to
10b4d7b
Compare
@@ -192,7 +97,8 @@ func TestForm_UnmarshalJSON(t *testing.T) { | |||
var got Form | |||
require.NoError(t, json.Unmarshal(data, &got)) | |||
|
|||
assert.Equal(t, form, got) | |||
dataGot, err := json.Marshal(&got) | |||
assert.JSONEq(t, string(data), string(dataGot)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this, equal is comparing pointers and not values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good just a few small thing mostly naming
8df7222
to
b00829d
Compare
web/src/stories/stepper.stories.mdx
Outdated
@@ -115,3 +140,160 @@ export const StepperStoryTemplate = args => ({ | |||
|
|||
<h2>Props</h2> | |||
<Props story = "Stepper component"/> | |||
|
|||
export const stepperHFDocs = { | |||
source: {code: `title = "Example Stepper with Horizontal Form" component.NewStepper()`}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a snippet of working code that reflects what is shown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
…epper Signed-off-by: Luis Enriquez <felipe096@gmail.com>
Signed-off-by: Luis Enriquez felipe096@gmail.com
What this PR does / why we need it:
Which issue(s) this PR fixes
Special notes for your reviewer:
For #2020
Vertical did not work so it was necessary to ask clarity team
Question to clarity team
https://vmware.slack.com/archives/C0JF8D2LB/p1619628404441100
Basically, there is no easy way to do it, is necessary to add a div with a layout horizontal for every row like is shown here
https://stackblitz.com/edit/typescript-ku7jz4?file=index.html
In order to encode that information on a JSON a new struct was created where all the horizontal field are contained on the attribute
field
#2178
The above solution did not work because label use a lot of space
So at the end it was implemented a solution using width
#2223
The solution proposed by @GuessWhoSamFoo basically changed
to this solution
This is how it looks right now
@GuessWhoSamFoo solution help to reduce the amount of code on form
Release note: