search

openpsa / jsgrid

Fork of last jqGrid version before license change
http://openpsa.github.io/jsgrid/
Other
28 stars 12 forks source link

inline delete impossible #79

Closed bouks closed 9 years ago

bouks commented 9 years ago

Bug in the confirmation modal (no button to validate or cancel)

Must merge before to see and fix this bug : https://github.com/bouks/grid.js/commit/9d3cbf427487e27220fab91a6d5ad1957db3f89b#diff-382b3649283911e1a5c64f3ec00e4968R1595

flack commented 9 years ago

Can this be closed? The referenced commit is merged

bouks commented 9 years ago

THis one, no, it's an actual bug on the modal.

"Still impossible to delete -> now bug in the confirmation modal (no button to validate or cancel)"

flack commented 9 years ago

Ah, ok. I must have overlooked the "modal" part :-)

bouks commented 9 years ago

I think the problem might be in all the delete (inline or not). Oleg made much modifications to modals. The problem certainly comes from his commits or meh-uk "imports". It should be fixed quickly, with no possible deletion, the grid is almost non-usable. @OlegKi @meh-uk you know this part of code, could you look at this ?

OlegKi commented 9 years ago

@bouks Thanks! It was my bug in the changes. It's fixed now.

bouks commented 9 years ago

We already have this fixed change here

Still buggy. No "cancel" or "ok" button in the confirmation modal.

OlegKi commented 9 years ago

@bouks Do you tried some demos which uses my code? For example try the demo or this one. I don't see any problems.

bouks commented 9 years ago

@OlegKi In your example the modal is too big, this is the same bug.

Fixed here. https://github.com/openpsa/grid.js/commit/e0f44aaf4a0d4e93c38eebe14ccc1874a0c8f825

flack commented 9 years ago

@bouks could you add some before & after screenshots? I've clicked the links, but it is not obvious to me what you mean? You can just drag and drop image files into the comment boxes here on github

bouks commented 9 years ago

before too-little-before after too-little-after

flack commented 9 years ago

Thanks! Now I get. After really looks much better.

Do you think it would also be possible to center the dialog relative to the grid?

bouks commented 9 years ago

Why wouldn't it be possible ? :)

I have no time for this right now. I just wanted to fix a critic bug.

So if no objection about this fix, i propose to close this issue (and then this one too https://github.com/bouks/grid.js/commit/9d3cbf427487e27220fab91a6d5ad1957db3f89b#diff-382b3649283911e1a5c64f3ec00e4968R1595) that is a critic bug and open a new one only for centering.

flack commented 9 years ago

the delete fix has already been merged here:

https://github.com/openpsa/grid.js/commit/9d3cbf427487e27220fab91a6d5ad1957db3f89b#diff-382b3649283911e1a5c64f3ec00e4968R1595

Your other commits are also already on master as far as I can see it. I don't have access to my dev machine right now, so I'm just going to trust you that they work :-)

If you have some sample code for testing this, it would be nice if you could add it as a new demo (especially since we have nothing showing the editing modes yet).

For future changes, it would be nice if you submit them as pull requests first, because that makes it easier to review. Stuff that changes documentation, build system or other auxiliary parts can be committed directly, but IMHO we should be extra careful when changing the actual JS code.

flack commented 9 years ago

Tested and verified fixed. I've added a simple inlineedit demo and filed a follow-up ticket for the popup rendering