Skip to content
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: (core) Add [state] to all input-group related components. Adapt forms #1613

Merged
merged 14 commits into from
Jan 6, 2020

Conversation

JKMarkowski
Copy link
Contributor

@JKMarkowski JKMarkowski commented Nov 18, 2019

Please provide a link to the associated issue.

fixes: #1085
fixes: #1217
fixes: #1216
fixes: #1721
fixes: #1715

It is WIP PR, just to summarize all the things done and start to make docs in breaking changes

Please provide a brief summary of this pull request.

There is new [state] @input on all of the input-group related component:

  • Combobox
  • DatePicker
  • DateTimePicker
  • InputGroup
  • LocalizationEditor
  • MultiInput
  • TimePicker

After some change on fundamental-style, Select component will be added to this list as well.

There is new documentation and usage of these component within Form, with fd-form-message component.

There is also new forms adaptation, which included change of

  • FormMessage component, the types to 'success' | 'error' | 'warning' | 'information'

  • FormControl component, now it supports the compact mode and has other types: 'valid' | 'invalid' | 'warning' | 'information'

  • FormLabel got new [checkbox], [radio] and [toggle] options, to define if it's used along with these input elements

  • InputGroup will have changed styling (no impact on NGX), but it will allow to use more than 1 addon one side.

  • Toggle now have semantic and compact modes.

  • Changed fd-form-set to fd-fieldset

  • Removed fd-input-group-search

Please check whether the PR fulfills the following requirements

Documentation checklist:

@netlify
Copy link

netlify bot commented Nov 18, 2019

Deploy preview for fundamental-ngx ready!

Built with commit 0aa77b6

https://deploy-preview-1613--fundamental-ngx.netlify.com

@JKMarkowski JKMarkowski force-pushed the feat/input-group-state branch 2 times, most recently from 08cf699 to 91ed595 Compare November 18, 2019 15:49
@JKMarkowski JKMarkowski changed the title WIP: Add [state] to all input-group related components. Adapt forms feat: Add [state] to all input-group related components. Adapt forms Nov 19, 2019
<br/>

<div fd-form-item>
<label fd-form-label>Disabled Date Picker</label>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disabled date picker still gets the hand on hover

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add :(core)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is added pointer-events: none no disabled popovers.

<br/><br/>
<div fd-form-item>
<label fd-form-label>Disabled Date Picker</label>
<fd-datetime-picker [state]="'information'" formControlName="disabledDate"></fd-datetime-picker>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cursor still has hand on hover

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is added pointer-events: none no disabled popovers.

Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2019-11-19 at 5 06 46 PM

Checkboxes look off

Screen Shot 2019-11-19 at 5 07 37 PM

The help icon should be on the right side of the input and there's no help message any more. It's been removed. Now it's just the default msg

@JKMarkowski JKMarkowski changed the title feat: Add [state] to all input-group related components. Adapt forms feat: (core) Add [state] to all input-group related components. Adapt forms Nov 20, 2019
@JKMarkowski
Copy link
Contributor Author

Hi @InnaAtanasova, did you check the preview checkbox? On my side it looks like that:
image
Also now help message is removed and inlineHelp flag is added to fd-form-label

@InnaAtanasova
Copy link
Contributor

It's radio buttons, not checkboxes that are broken

@droshev droshev added this to the sprint 24 - Maester Aemon milestone Nov 21, 2019
@droshev droshev added this to In progress in Development via automation Nov 21, 2019
@droshev droshev added the enhancement New feature or request label Nov 21, 2019
@JKMarkowski
Copy link
Contributor Author

JKMarkowski commented Nov 22, 2019

@InnaAtanasova
The radio's issue comes from fundamental-styles lib. It's about some weird order of css properties.
All of the changes from 0.2.0 has been introduced.

@droshev
Copy link
Contributor

droshev commented Nov 30, 2019

@JKMarkowski @InnaAtanasova @stefanoScalzo are we waiting for anything more in this PR?

@JKMarkowski
Copy link
Contributor Author

Hi @droshev, We are waiting for SAP/fundamental-styles#442 I think.

@JKMarkowski
Copy link
Contributor Author

Newest 0.4.0-rc.21 is adapted on this PR.

<span class="fd-toggle"
[ngClass]="
(size ? ('fd-toggle--' + size) : '') +
(semantic ? ' fd-toggle--semantic' : ' ') +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should not be a space in between the quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be quotes, because it's treaten as a string, without empty string it will be
fd-toggle--sizefd-toggle--semantic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quotes yes but there is a space on line 5 in the single quotes

@@ -1,7 +1,10 @@
<fd-input-group [compact]="compact"
[state]="state"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a state on the input group and the addon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise it's not applied to inputs.

max-height: 100%;

.fd-input-group__addon--button .fd-input-group__button {
&:hover {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is on styles, but on NGX we have some issues with classes hierarchy, so it's mandatory to add it there aswell.

'fd-input-group--compact' : compact}">
[ngClass]="(inline ? 'fd-input-group--inline ' : ' ') +
(disabled ? 'is-disabled ' : ' ') +
(state ? 'is-' + state : ' ')">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also be written like this
[ngClass]="
{
'fd-input-group--inline': inline,
'is-disabled ' : disabled,
is-' + state: state
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It throws error, there is no way to add dynamically class as key in ngClass object

@@ -4,3 +4,5 @@
@import "~fundamental-styles/dist/radio";
@import "~fundamental-styles/dist/textarea";
@import "~fundamental-styles/dist/form-select";
@import "~fundamental-styles/dist/fieldset";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@InnaAtanasova Not sure if this should be importing all of these styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed unused.

@JKMarkowski
Copy link
Contributor Author

@stefanoScalzo All your comments has been resolved, stackblitz is working. Please take a look again, so we can merge it.

Development automation moved this from Review in progress to Reviewer approved Jan 6, 2020
@JKMarkowski JKMarkowski merged commit adabbc7 into master Jan 6, 2020
Development automation moved this from Reviewer approved to Done Jan 6, 2020
@JKMarkowski JKMarkowski deleted the feat/input-group-state branch January 6, 2020 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants