-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: forms refactoring #419
Conversation
Deploy preview for fundamental-styles ready! Built with commit 6a90a95 |
@@ -15,7 +15,7 @@ | |||
|
|||
> .fd-tile__content { |
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 those stylings should be even removed. They create false expectations
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.
src/input.scss
Outdated
|
||
&:last-child { | ||
margin-bottom: 0.25rem; | ||
} | ||
|
||
&::placeholder { | ||
@include fd-reset(); | ||
@include fd-var-color("color", $fd-forms-color--placeholder, --fd-color-neutral-4); |
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.
can we replace the old values with theming parameters?
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
src/mixins/_mixins.scss
Outdated
@@ -6,7 +6,7 @@ | |||
|
|||
@include fd-var-color("color", $fd-color, --fd-color-text-1); |
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.
can we replace with theming parameters?
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
a248e3b
to
ca5f7a3
Compare
@InnaAtanasova JFYI: the forms documentation still refers a lot to "form groups". But the form groups have been removed. |
&--inline { | ||
float: left; | ||
margin-right: fd-space(4); | ||
&--horizontal { |
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.
What are the different use cases for --horizontal
versus --inline
?
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.
--inline
is what we had before for displaying the checkboxes and radio buttons inline.
This was only the elements go to one line, the label is on another line.
But then we needed to add inline items - label and input inline. This was achieved by using flexbox.
Stefano is working currently on Forms Layout - there's going to be vertical and horizontal layout.
So, to minimize the breaking changes, we decided to keep the --inline
modifier for displaying the checkboxes and radio btns inline, and add --horizontal
for displaying the items inline
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 just had a quick question about the need for --horizontal
versus --inline
but other than that looks great to me! 🚢
Thanks for pointing this out. You are absolutely right. We will def improve the documentation. |
Description
Refactored:
fd-form-label
- added--required
modifier for styling the*
fd-form-item--horizontal
modifier class to display form items inline (horizontally)fd-form-label--inline-help
for form items with inline helpfd-form-message--help
-BREAKING CHANGE
fd-textarea
- added interaction states, compact modefd-radio
- added interaction states, cosy and compact modefd-checkbox
- added interaction states, compact modefd-form-label--radio
,fd-form-label--checkbox
andfd-form-label--toggle
modifier classes to labelScreenshots
Before:
After:
About tests:
Running locally the following tests failed. They have been updated