Forms variables removed

[nastiatikk]

Status: In process Reviewers: @kpeatt @jeffkamo @avelinet @ry5n @mlworks @cole-sanderson @yourpalsonja

Changes

• All $forms__variables were removed from forms.scss and replaced with actual value.

  Todos:

• [x] Should be additionally reviewed after merging Neutral Variables PR https://github.com/mobify/vellum/pull/79
• [x] Designer +1
• [x] Designer +1

[ry5n]

We can probably retain some of these variables but just move them into the main _variables file. This is what I’d retain:

$input-border-color: $border-color;
$input-background-color: white;
$input-focus-outline-color: blue; // value TBD
$input-horizontal-padding: $unit;
$input-vertical-padding: $unit;

// We might also need the disabled color variables but would like to find some 
// way to avoid needing three of them

[nastiatikk]

This was my intention. Though don't know if $input-border-colour should be there, as it's a border-colour itself. Though for better collaboration with select it will probably make sense to have it as a variable…

[nastiatikk]

I probably added more variables, but I find them all useful if we're gonna use them with Select component.

$input-height: 40px;
$input-vertical-padding: $unit;
$input-horizontal-padding: $unit;
$input-background-color: white;
$input-border-radius: $border-radius;
$input-focus-outline-color: $accent-color;

I didn't use $input-border-color because we have $border variable to achieve the same goal.

Also swapped :active and :focus for General form elements, because :active state was overwritten by :focus and was never visivle

[ry5n]

After looking at this again, I think I was getting ahead of myself. This PR should be to fix #75 which is about the forms__ variable seeming like they are for a form component, and moving them to the main _variables file. I made a mistake suggesting that we change the names and add/remove variables, either that or I copy-pasted the wrong thing.

In other words, we should just rename these to the main file and renaming from e.g. forms__border-color to forms-border-color. @nastiatikk does that make sense (I am happy to fix this up since it’s my own bad advice.)

[nastiatikk]

You think we should remain all those 15 form variables and just rename them? Without refactoring them to smaller and more useful amount? If we're gonna do it in future why not do it now since this work has already been done and time spent?

[ry5n]

For now, yes, just the rename. I definitely want to simplify things somewhat but there are two reasons I don’t want to do it here:

1. It’s not what this issue is about (it can be hard, but it’s worth keeping issues small and focused).
2. Stencil is probably going to lead to further variable revisions, so we should do them together if possible.

[nastiatikk]

I see you point and agree with it. But from the time perspective it looks like a waste of it. There was a ton of conflicts from merging-from-master, because after all the latest merges and variable renames _forms.scss was almost 100% changed. I spent maybe 2 hours on resolving those conflicts. I see it was my only mistake doing these changes in this PR and I can undo them all, no problem at all. But I was thinking since time was spent maybe we cad add some more effort to finish the renaming and cleaning part within this PR

[ry5n]

I totally understand. It’s my bad for suggesting adding/removing variables here, I apologize for the wasted time :( But I think we should still stick with the initial intent of the issue and PR. Let me know if you’d like me to pitch in some commits so you don’t have to spend more time.

[nastiatikk]

No worries, I will revert these styles myself =)

[ry5n]

Thanks.

[ry5n]

+1, Looks great.

[ry5n]

Looks perfect.

[tedtate]

:+1: :shipit: