CityWebConsultants / Iris

Modular content management and web application framework built with Node.js and MongoDB
http://irisjs.org
Other
9 stars 7 forks source link

various tidy up fixes and password strength helper #256

Closed adam-clarey closed 8 years ago

adam-clarey commented 8 years ago

This turned into a slightly bigger commit than i expected.

There are some minor styling updates to the base theme.

It no longer shows commas in regions where no blocks exist.

Blocks are now wrapped in divs with their id, although i think this can be improved. It's currently not possible to change block titles as they are used as the unique identifier. For custom blocks maybe a random string can be used for their unique identifier so that that titles can be edited.

I've also added a password checker to improve security, to implement, if you have a form with 'password' field, you just need set its type as 'password-confirm' ala drupal.

data.schema.password = {
  "type" : "password"
};

data.form = [{
  "key" : "password",
  "type" : "password-confirm"
}];

The rest is taken care of.

screen shot 2016-05-15 at 18 52 34

It also tells you whether the 2 passwords match.

As a professional system, this kind of functionality would be expected.

I also have some dev code still in there for the hopes someone can help figure out a problem I was having. Currently the password matching isn't properly validated, its just a visual cue. If you submit the form when they don't match it will still submit. I was trying to work Jsonforms clientside validation into it to prevent submission.

As a happy accident of this, its now possible to add clientside validation to your form definitions. So for instance:

data.validate = {
  "validate" : function (values, schema) {
    return [{ "message" : "some error message"}];
  }
}
``
This will prevent the form submission and show the error message.
FilipNest commented 8 years ago

Hiya!

General

Could this sort of thing be split into several pull requests? Especially if they're not dependent on one another it makes it a lot easier to reference when things go wrong at a later date as they're all archived.

Blocks

Adding a div in the blocks template is fine (I personally don't like extra divs and forced classes as this should be down to the block itself but I suppose you can always overwrite the regions template so fine with it). For block instances, I'd simply recommend when it's added to a region adding a unique ID to it there on render that you could use in a template if you want. Most of the times you'll be able to get to it with CSS from the region it's in but there's no harm in an instance identifier in the data for use if desired.

Passwords

I like the password duplicate checker but think this is a major overkill for what we're trying to achieve. The bootstrap-strength-meter js file I suppose is fine as it's small but the password score library is 36.8MB which is only 4MB less than all of Iris plus all its dependencies (the majority of that is the 11MB prettydiff module which I'm going to remove for https://github.com/CityWebConsultants/Iris/issues/216 so this will be much bigger than the whole thing). I also don't think it's core functionality. Especially as password strength is something with lots of different opinions about it. Length is the only important issue but even that should be down to the site owner to advise on.

I'd recommend moving the password strength meter into contrib for people who really need it. The password-confirm box is a good idea but could probably be handled by two password fields and validation to check they're the same without having to define a new field.

Happy for the new field itself as it's a minor addition, however it should be defined like the rest of the extra fields with the registerWidget function documented over at: https://github.com/CityWebConsultants/Iris/wiki/Form-system#extending-json-form-with-custom-fields rather than placed in clientforms.js . This way it's extendable, overwritable and also, eventually once we build in that functionality, will only be used if needed.

For the client side validation system you've put in, are we using JSONForm's own form validation system for this? https://github.com/joshfire/jsonform/wiki#validation there are events before the form goes to the submit handler and others. Might be worth looking into as we should stick to JSONForm's documented system as much as we can.

Trailing commas

So glad you fixed this, I should have done this ages ago!

Styling

All looks fine, regarding form styling we should look at how any custom elements we put in work without bootstrap so they can be used on non admin pages without issues.

adam-clarey commented 8 years ago

Ive removed the password confirm from this PR. I'll submit that separately.

This just contains theming. For clientside validation, it can now use the jsonform validation.

As mentioned above, when defining a form just use the syntax:

data.validate = {
  "validate" : function (values, schema) {
    return [{ "message" : "some error message"}];
  }
}

To make use of it.

The block id issue can be another PR

FilipNest commented 8 years ago

Awesome.