Skip to content
This repository has been archived by the owner on Jan 1, 2020. It is now read-only.

Oleg gridjs #50

Merged
merged 15 commits into from
Jan 13, 2015
Merged

Oleg gridjs #50

merged 15 commits into from
Jan 13, 2015

Conversation

meh-uk
Copy link
Contributor

@meh-uk meh-uk commented Jan 13, 2015

Applying a whole bunch of Oleg's changes to the main branch.

…"local"

Usage of `jsonmap` property can be helpful to read local data (the data
from an object) without the requirement to normilite it.
http://www.w3.org/TR/wai-aria/states_and_properties#aria-multiselectable
describes elements which can have aria-multiselectable attribute.
Element with the role="presentation" can't have it. Correct location is
the element which have role='grid'. It's `<div class='ui-jqgrid-view'>`
in jqGrid.
jqGrid don't set any font-size on gbox (.ui-jqgrid), so the usage of
$(".ui-jqgrid").css("font-size") can be wrong.
The align attribute on the td element is obsolete. One can set
text-align attribute on the cell instead and, if the child element is a
block, to set margin-left:auto or margin-right:auto or both on the child
block element
One used `dtbl` variable **always** as `"#"+dtbl`. So we change the
value of `dtbl` from `"DelTbl_"+$.jgrid.jqID(gID)` to
`"#DelTbl_"+$.jgrid.jqID(gID)` and replace `"#"+dtbl` in the later code
to `dtbl`.
flack added a commit that referenced this pull request Jan 13, 2015
@flack flack merged commit 510f6e1 into openpsa:master Jan 13, 2015
@flack
Copy link
Contributor

flack commented Jan 13, 2015

Great job! I'll take a look and see if I can merge some more.

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

Have yout tested it ?

Nothing work anymore here. In my projects ( dev of course :) ) and my examples, like :

http://ribouka.com/test/

When i click IN the grid there's a lot of messages in console like :

"click in column resizer: pageX is modified from undefined to 302"
or
"GBOX: dblclick in column resizer: pageX=525;525, idx=undefined, e.pageX=525, cm.name=null"

@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 13, 2015

I didn't have time to test it unfortunately :(.

@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 13, 2015

Sorry I should have made that clear.

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

I think all these merging should have been made by Oleg if he wants to join us, nobody else.

We have encountered the same problem (but minor because less sensible and less commits) with search toolbar when flack re-did my changes.

Everybody should commit their only changes with minor tests also.

@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 13, 2015

I think in retrospect you're right.

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

One could test and commit a little change seen on a fork, no problem. But mass import should be avoided. :)

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

Ok it seems working.
With recent autodetection, datatype was not needed anymore (for remote data).
Now it is needed again. That's why it was not functioning here.

@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 14, 2015

Lets be clear though that my objection to merging any more of @OlegKi's changes is entirely practical and not at all ideological. Unless Tony has made far fewer improvements than we have in recent weeks I can't believe it is likely to be possible to merge Oleg's changes into the main branch of jqGrid either. We have been trying pretty hard to make merging the branches as easy as we can.

And when it has been near on impossible to merge changes into the main branch of jqGrid for so long it shouldn't be at all surprising that there have been a lot of changes in a short time to this fork.

@flack
Copy link
Contributor

flack commented Jan 14, 2015

Well, the thing is that Oleg is the most active developer (even more than Tony), and many of his changes are pretty useful, so it would be a shame if we had to duplicate the effort and implement them again manually against our code base.

The good news is that we are currently merging faster than he commits. We were almost 80 commits behind when we started merging, and now we're down to approx. 30 (and ca. 10 of them are README changes). So if possible, I would like to close that gap, and then talk to Oleg again about switching his repo to fork from ours. If he says yes, then all is well. If not, then I guess we will have to go our separate ways, but like I said, IMO it would be a complete waste of resources (not to mention confusing to users) to maintain two different forks of the same codebase.

@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 14, 2015

Two forks would definitely be a shame.

@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 14, 2015

OK I've made some more progress, there is just one change from the 13th and the changes from the 14th to merge. Plus all the CSS changes which have been skipped.

@flack
Copy link
Contributor

flack commented Jan 14, 2015

there also seem to be some commits from the 11th which we haven't merged yet:

or are they somehow squashed into other commits?

@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 14, 2015

No. I'm incompetent and missed them.

@flack
Copy link
Contributor

flack commented Jan 14, 2015

Don't be too hard on yourself. I'm sure it was just your subconscious that was trying to protect you from the pain that is manual merging :-)

@meh-uk meh-uk mentioned this pull request Jan 14, 2015
@meh-uk
Copy link
Contributor Author

meh-uk commented Jan 14, 2015

Added those missing commits to #57.

@meh-uk meh-uk modified the milestone: 1.0 Feb 7, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants