xogroup / tk-form-fields

3 stars 1 forks source link

[WIP] 3.0.0 #19

Closed GeorgeTaveras1231 closed 7 years ago

GeorgeTaveras1231 commented 7 years ago

Changes

BEM style class declarations.

This is just a matter of preference that is negotiable. This is also inspired by a project Acquisition is currently working on where we are only using BEM, and it felt wrong to introduce none BEM classes

Introduce field container

Reason: The label position worked previously because in the demo page, we were using .col-x-x divs as containers for the fields. These containers were serving as the anchor for the label's absolute position, they were also providing some left-padding that allowed the labels to look fine. When the column classes are removed, the fields look wrong. This change is to decouple the way the field looks from the grid system.

<div class="tk-field__container">
<input id="email-field" type="email" name="email" placeholder=" " />
<label for="email-field">Email</label>
<div class="tk-field__requirements">Field validation message</div>
</div>

From fieldset to tk-field__group

Reason: Allow for logical grouping of fields based on business logic using fieldset while allowing the freedom to visually group any fields.


<!-- Example with two fields -->
<div class="tk-field__group">
<div class="col-xs-6"><!-- field markup omitted --></div>
<div class="col-xs-6"><!-- field markup omitted --></div>
</div>

### Introduce dropdowns
> This is per an **Acquisition** requirement. Dropdowns can be created with the following markup:

```html
<div class="tk-dropdown__container">
  <input id="season" readonly="true" placeholder=" " name="season">
  <label for="season">Season</label>
  <span class="tk-dropdown__caret"></span>
  <div class="tk-dropdown__list-container">
    <ul class="tk-dropdown__list" id="season" name="Season" role="dropdown">
      <li class="tk-dropdown__item tk-dropdown__item--is-focused" data-value="Winter">
        Winter
        <span class="tk-dropdown__item__check"></span>
      </li>
      <li class="tk-dropdown__item" data-value="Spring">
        Spring
        <span class="tk-dropdown__item__check"></span>
      </li>
    </ul>
  </div>
</div>

Lightweight theme system

Themes can be enhanced by extracting the 'theme-able' properties from the modules at src/stylesheets/placeholders/ and adding them to the tk-form-fields-theme mixin at src/stylesheets/_themes-generator.scss. Theme definitions currently live on src/stylesheets/_themes.scss

%tk-form-fields--new-theme {
  @extend %tk-form-fields;
  @include tk-form-fields-theme($base-color: $white, $accent-color: $gray-95 );
}

Export themed placeholders

Currently only includes two themes. %tk-form-fields--default and %tk-form-fields--white. (Open for name improvements)


// Use the light theme if your form is in a knottie-blue (or new-blue) background
.my-form-is-in-knottie-blue-background {
@extend %tk-form-fields--white;
}

// Use the default theme otherwise .my-form-is-in-white-background { @extend %tk-form-fields--default; }


### Extracted tk-buttons

> These now live in [sharedweb/tk-buttons](https://git.xogrp.com/sharedweb/tk-buttons/pull/1)

TODO:
===
- [x] Update demo
- [x] Use consistent names for class selectors.
- [x] Figure out what to do with buttons
- [ ] Update js tests
- [ ] Update documentation

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/xogroup/tk-form-fields/19)
<!-- Reviewable:end -->
pkelly commented 7 years ago

Buttons lost their styling. I see you have a todo above for it, just wanted to call it out in case that is for something else. screen shot 2016-11-21 at 4 37 43 pm

pkelly commented 7 years ago

@GeorgeTaveras1231: Do you get the below JS error on dev? Not sure if there is some other setup I need to do other than npm install (index):1 Uncaught ReferenceError: XO is not defined

pkelly commented 7 years ago

@GeorgeTaveras1231: Overall this looks great to me πŸ‘ I added a few comments above.

GeorgeTaveras1231 commented 7 years ago

@pkelly I took some of your advice regarding naming and fixed the buttons. I also updated the documentation of the PR above. This has now become a lot more lengthy but I am happy to assist with the review process in whichever way I can. As far as the JS error you pointed out, this is because the server doesn't build the JS by default, that is something we should add. In the meantime just make sure to run npm run build before viewing the demo page.

pkelly commented 7 years ago

@GeorgeTaveras1231: Good call on the npm run build Not sure how I missed that in the README πŸ˜…

I pulled the latest on this branch, but am getting an import error.

$ npm start

> @xogroup/tk-form-fields-pattern@3.0.0-0 start /Users/pkelly/work/tk-form-fields
> node index.js

TK Form Fields dev server listening on port 9494
  source: /Users/pkelly/work/tk-form-fields/src/stylesheets/index.scss
  dest: /Users/pkelly/work/tk-form-fields/public/index.css
  read: /Users/pkelly/work/tk-form-fields/public/index.css
  error: File to import not found or unreadable: @sharedweb/tk-typography/dist/latest/tk-typography
Parent style sheet: node_modules/@sharedweb/tk-buttons/dist/latest/tk-buttons.scss

in node_modules/@sharedweb/tk-buttons/dist/latest/tk-buttons.scss:3:1
Error: File to import not found or unreadable: @sharedweb/tk-typography/dist/latest/tk-typography
Parent style sheet: node_modules/@sharedweb/tk-buttons/dist/latest/tk-buttons.scss
    at options.error (/Users/pkelly/work/tk-form-fields/node_modules/node-sass/lib/index.js:283:26)

Since this PR has gotten pretty huge I would suggest blocking any more major changes so that it can get merged in sooner than later.

GeorgeTaveras1231 commented 7 years ago

@pkelly Thanks for looking at this again. That error is a result of the dependency issue I pointed out in that thing I wrote. I'm looking into it now.

GeorgeTaveras1231 commented 7 years ago

@pkelly I ran the server locally and it worked fine.

pkelly commented 7 years ago

@GeorgeTaveras1231 helped me out with that error. I had to clear my node_modules. Works great now πŸ‘

GeorgeTaveras1231 commented 7 years ago

Just an FYI this PR hasnt been merged because all of this code is likely to change once the distribution/package architecture of the union project is established