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(form): Added styles for an invalid input #121

Merged
merged 4 commits into from
Jul 25, 2018
Merged

Conversation

fragsalat
Copy link
Contributor

By adding the class .is-invalid to the input, the border and icon will switch to an error color.

screenshot from 2018-07-25 11-21-14

Copy link
Contributor

@MihaChicken MihaChicken left a comment

Choose a reason for hiding this comment

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

Should we reflect this changes somewhere in docs?


&.icon24 {
margin-top: -12px;
}
}

input.is-invalid + .icon {
Copy link
Contributor

Choose a reason for hiding this comment

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

using + selector looks hacky to me. Could lead to some unexpected behavior.
Can we somehow avoid it?

Copy link
Contributor Author

@fragsalat fragsalat Jul 25, 2018

Choose a reason for hiding this comment

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

Why does it look hacky to you? It's a common selector and it just affects the icon next to the input. I wouldn't know a better way to write this and right now I don't know why this is hacky^^ Would be interesting if you find a better way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because selector is referring to any icon which is placed next to input, but not every icon which placed next to input should be painted red when input is invalid. For example if it is question mark icon next to the input to clarify what is actually input value stands for.

Also not in all cases icon which goes after the input is actually referring to this input.
It's probably an edge case but still let's take it as an example:

<input name='somedata' style='float: left'/>
<span class='icon icon-dropdown'  style='float: right'> some another dropdown icon to show not related data</span>

In this case both of this UI elements will be located on different sides of the screen and will play different roles.
And in the end this is a selector which can not be autoprefixed which leads to a problems I already described before.

About better ways:
If we really want our UI system to support inputes combined with icons then we should probably introduce a wrapper for them like bootstrap and foundation do.
If combining icon with input is not such common case it's maybe doesn't worth to add such feature to UI library but instead implement it inside app which needs it.

Copy link
Contributor Author

@fragsalat fragsalat Jul 25, 2018

Choose a reason for hiding this comment

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

Ehm there is a wrapper class called .input-wrapper. Only when this class is grouping an icon and an input this rule applies. I think this is a pretty good pattern to on the one side allow freedom to the developers but on the other side give a guideline how to display things.

<div class="input-wrapper">
  <input class="is-invalid"/>
  <span class="icon icon-cross"></span> <!-- Here should be no other icon in this case! -->
</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

@fragsalat sorry my bad, you are right, haven't noticed this 🐌
Then it's not such a big deal but still the thing I would like to see in fabric css is when I'm putting class on some tag then it will change this tag style but not style of it's neighbor.
It's like explicit over implicit. In this case I would expect .is-invalid class on wrapper not on input itself.

.input-wrapper.is-invalid .icon {
   color: red;
}

Looks much more explicit for me

But as I said not such a big deal now... Approving this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fun fact I already had it like this but changed it for any reason. The good thing is that nothing is fixed and everything can be refactored :)

@fragsalat fragsalat merged commit 5bca6fa into master Jul 25, 2018
@fragsalat fragsalat deleted the invalid-input branch July 25, 2018 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants