[DT] Added isHidden method to determine if a cell or row is visible #1243

Open
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

apipkin commented Sep 27, 2013

Revamp of PR #681

  • Test
  • Code Fix
  • History
  • Sign Off or 3 days

For the cell editor, I had to hide the editor when the cell being edited is scrolled away. The code to determine if the cell is still visible relies too heavily in the particulars of the way scrolling is implemented so it could not reasonably remain with the cell editor with such a strong dependency.

The only public method that datatable-scrollable currently offers is scrollTo. I added isHidden to determine if the cell or row is visible or not.

To do so I broke apart the part of the code from scrollTo that locates the element to be scrolled to and put it in a shared _locateTarget method.

isHidden takes the same argument as scrollTo plus a partially flag to allow to ask whether the element is totally hidden (the default) or just partially covered.

I am not sure about the names. Should I provide isHidden or isVisible ? Perhaps it should be isElementHidden (it handles cells and rows so how could I refer to either?). Also, _locateTarget got that name because the local variable in scrollTo was called target but I can't claim the name is a good one. Suggestions are welcome.

While adding tests I noticed that _locateTarget could not locate a row by the clientId of the model it represents as the API docs claims so I added that.

I am also unsure of whether to provide that partially argument to isHidden is the right thing to do or if it would be better to provide different returns, for example, false or 0 for not hidden, true or 2 for totally hidden and 1 for partially hidden (which also evaluates to true if you are not particularly concerned). Or 0, 1 and -1.

I also found that scrollTo doesn't really scroll fully into view a cell beyond the right edge when there is a vertical scroll bar. scrollIntoView ignores that the vertical scroll bar is there so the cell gets at least partially hidden behind it. It makes isHidden fail because on all of the other edges, if you scroll to a cell, it gets completely visible while in this case, it gets at most partially visible.

I wonder if providing a region (like node.get('region')) would be a good idea.

Satyam and others added some commits Apr 30, 2013

Added isHidden method to determine if a cell or row is visible
Took the part of scrollTo that locates the target into a separate
_locateTarget method to be shared with isHidden.
Fixed _locateTarget which did not locate by the model clientId as
stated.
Added tests for both scrollTo, isHidden and _locateTarget.
Simplified the scroll event. Added scrollTop and scrollLeft attrs
Dropped the extra arguments in the event payload.
Added scrollLeft and scrollTop attributes, both can be get or set.

Conflicts:
	src/datatable/js/scroll.js
	src/datatable/tests/unit/assets/datatable-scroll-tests.js
ScrollTo now does not use scrollIntoView
Removed the call to DOM's scrollIntoView which behaves funny,
and switched to do the calculations.
Added a read-only scrollRegion attribute that returns an object
like Node.get('region') but for the scrolling area.

Conflicts:
	src/datatable/js/scroll.js
Owner

ericf commented Sep 27, 2013

@apipkin this is going to miss Sprint 10, right?

Contributor

apipkin commented Sep 27, 2013

Unless I get sign off ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment