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

Grid form #657

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Grid form #657

wants to merge 4 commits into from

Conversation

manuelnaranjo
Copy link

This series of patches adds support for multiple fields in a row as suggested by #448

It's using bootstrap for generating the needed layout.

A configuration example would be:

   var datosPersonales = nga.field('Datos Personales', 'fieldset').
    rows([
      // apellido y nombre
      nga.field(null, 'row')
        .fields([
          nga.field('apellido').attributes({span: 3}),
          nga.field('nombre').attributes({span: 3}),
          nga.field('grupo').attributes({span: 3}),
          nga.field('ficha').attributes({span: 2}),
          nga.field('activo').attributes({span: 1})
        ]),

      // datos de la escuela
      nga.field(null, 'row')
        .fields([
         ....
        ]),
      .....
  ])

Now that admin-config allows settings css class for the label, let's
honor it.
Adding basic support for fieldsets and rows in the create, edit and show
views.
Adding necessary css classes modifiers for a grid layout, special care
needs to be taken with date inputs, as those are using col-sm-4 by
default which doesn't work nice inside a grid form.

More fields may need to be fixed, by now I'm only fixing those
Adding necessary changes to views so grid-forms can be used.
@fzaninotto
Copy link
Member

Great!

I'm not a big fan of nga.field(null, 'row'). A row isn't a field, so you should provide a nga.row() factory instead.

Also, please update the package.json to point to your admin-config fork for the time being so that the tests have a chance to pass. By the way, you should test this.

Lastly, with every new feature comes a bit of documentation... please contribute that part, too.

@manuelnaranjo
Copy link
Author

Mhh the nga.row suggestion makes sense, I'll do that, same for fieldsets, those aren't fields either.

Ok I will change package.json, I haven't though about it before.

Where should I put the docs? Suggestions?

@fzaninotto
Copy link
Member

Probably in doc/Configuration-reference.md

@michaelavi
Copy link

It's an awesome work, can you help me why I got 'Unknown field type "fieldset".' error? I've changed my ng-admin as shown on your Files changed, but still got that error....

@manuelnaranjo
Copy link
Author

@michaelavi are you using webpack? If not you need to switch to it, as I haven't rebuilt the files on my own branch

@Phocea
Copy link
Contributor

Phocea commented Nov 18, 2015

Sorry for bumping this, but is this feature is ready to be pulled or needs more work ?

@manuelnaranjo
Copy link
Author

I've been using it for quite sometime, but the branch needs some cleanup and testing

@Phocea
Copy link
Contributor

Phocea commented Jan 13, 2016

@manuelnaranjo Any chance of finalizing this? Its a it of a shame that it is left so close to bring some good functionality to ng-admin
@fzaninotto Can you share your opinion on what is still mising to finalize this pull request?

@fzaninotto
Copy link
Member

see my comments in the discussion

@jefbarn
Copy link

jefbarn commented Apr 6, 2016

+1 for this.

@manuelnaranjo
Copy link
Author

I'm sorry but I don't have the time right now on fixing this for merging, maybe someone can take over? I can help out, but I can't do my self.

@jefbarn
Copy link

jefbarn commented Apr 7, 2016

Took at quick look at the merge, but the gotoDetails() and gotoReference() changes make a lot of conflicts.

@mohamed5678
Copy link

mohamed5678 commented Jun 7, 2016

@michaelavi did you manage to make it work? I'm having the same issue

@thachp
Copy link

thachp commented Dec 1, 2016

@fzaninotto Please merge this feature.

@Phocea
Copy link
Contributor

Phocea commented Dec 1, 2016

@thachp it is not possibile in the current state:

  • Merge needs a review, in the meanwhile @fzaninotto added the possibility to have template on labels using a compile attribute, but this remove the class attribute. It is needed to verify if that class is required for this PR. Maybe @manuelnaranjo could have a quick look and let us know the impact

  • PR still need some work as per @fzaninotto previous comments (documentation and nga.row factory in the admin-config code)

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.

7 participants