eddyystop / mithril-components

Components, mixins, patterns and sample code for mithril (lhorie/mithril.js)
MIT License
114 stars 3 forks source link

components doing occlusion #4

Open eddyystop opened 10 years ago

eddyystop commented 10 years ago

From Satyam: I like the occlusion control, though its name is somehow cryptic, at least to me.

I noticed in the description of the controller it lists contentsController twice, the second time should be contentsView.

I like the idea and I'd love to put it into DataTable though is not so easy as the headers have to remain in place while the body scrolls and the vertical dividers have to match. Pagination is easier to do and I was planing on doing that shortly.

There are not many scrollable things other than a list I can think of. Thus, I am wondering whether it might be easier to have a, say, scrollingList or infiniteScrollingList component so that the developer doesn't need to implement the list part of it.

With that in mind, I think I would provide the controller of the component with an itemProvider function that would produce the items to be displayed as they are requested, and another function (possibly a property getter/setter) that returns the length of the list.

The component would initially request, say, the first 10 items (this can be configured) and calculate the height of the items based on the actual height of those 10 items. It would immediately do a second request if those initial 10 items don't get to fill in the container height.

I don't like giving the item height via code as a later change in the CSS might mess things up. Checking how tall they actually are seems safer. Even the end user might have his/her browser configured with a larger font and the items in the list would look cramped in a fixed-size container. I remember a finance guy in a company I worked for that had what in those days was a huge screen to have his enormous spreadsheets, and he used a very small font to make it all fit in. He would have hated to see the items take up so much waster space.

The component should check the length of the list regularly as the length might well be an estimate (like google's search, the number of 'oooo's in the colorful Gooooooogle' at the bottom of the page is just an estimate). The developer might load batches of items from the server and the size and position of the scroll bar index would change accordingly.

A falsy (null, undefined or 0) length would mean the list might go forever.

I'd like to suggest to open new issues instead of keep adding to this item, where the conversation would get lost and others might skip on it for lack of a decent title. So, I'll just close it.

eddyystop commented 10 years ago

I'll push the new occlusionContainer and occlusionList in a few hours.

eddyystop commented 10 years ago

I pushed a new occlusionController and occlusionList. An item's height is computed automatically. The scroll bar is fixed at first render by forcing a rerender which doesn't have much to do to the DOM.

I think you're right that occlusionController should be absorbed into each occluding component. It would be easier to understand.

I'd dislike repeating getComputerSizes() in each component. Even repeating the m.render's annoys me. We'd have to made modifications to every component if we find a bug. Perhaps we should start a util file with getComputerSize() and the extend() which is used in multiple components.

eddyystop commented 10 years ago

Dropped occlusionControl, pushed new occlusionList. Much better.

Satyam commented 10 years ago

The problem with a table is that the TBODY doesn't allow for much styling or other properties, it is quite a dumb container. There is no way to set a height for the TBODY, set scrollTop or attach scrolling bars independently of the other table sections.

I've seen solutions where the scrolling datatable was actually made of two tables, one with the headings, the other with the contents, with its columns kept aligned via code. The table containing the data, being a completely separate table, could have independent scrollbars simply because it was a completely independent table.

eddyystop commented 10 years ago
  1. Run the new occlusionTable.html to see what I'm suggesting. I think its a better approach than that 2 table approach I too have suffered with. The column widths in the example are defaults and they change when you hit row 100.
  2. I'm considering implementing horizontal scrolling now. However to keep the code manageable perhaps the params should specify the number of columns to display rather than the component calculating a possibly changing table width. We could render more columns than needed but is that so terrible? Would do you think?
Satyam commented 10 years ago

On 26/06/14 14:37, Eddyystop wrote:

  1. Run the new occlusionTable.html to see what I'm suggesting. I think its a better approach than that 2 table approach I too have suffered with.

It does look good! I really don't remember why there was such a fuzz about making the table scroll smoothly. Scrolling a full record at a time looks pretty good. After all, the mousewheel does not roll smoothly, it is jumpy on purpose.

The column widths in the example are defaults and they change when you hit row 100.

  1. I'm considering implementing horizontal scrolling now. However to keep the code manageable perhaps the params should specify the number of columns to display rather than the component calculating a possibly changing table width. We could render more columns than needed but is that so terrible? Would do you think?

If at all possible, the dimensions should come out of the CSS styling of the page. Dimensions in code should only be an alternative to whatever dimensions come out of the CSS layout. If, at any time, the layout needs to be changed, editing the CSS should sufice. With responsive grids such as those provided by Bootstrap or PureCSS, it is better to let the CSS rule.

— Reply to this email directly or view it on GitHub https://github.com/eddyystop/mithril-components/issues/4#issuecomment-47219976.

eddyystop commented 10 years ago

I've been working with col widths. They aren't fixed in general for a table. The issue is you may determine you haven't rendered enough columns and you don't yet know the widths of the following columns. You may find its not enough to add just 1 column. And what's the advantage of find the exact number of cols because circumstances may totally change next scroll.

Occlusion is about rendering fast and perhaps any jockeying costs more than rendering a larger-than-necessary but safe number of columns. Hence perhaps the params should specify that number.

Perhaps the params can indicate if the header widths are fixed. In this case, we could compute all the column widths with one 'test' render and use it for the entire table.

Satyam commented 10 years ago

Tables can be infinitely long but they are finite in width. Even with spreadsheets, where there is no preferred dimension, sheets tend to be longer than wide. I wouldn't trouble myself trying to save rendering time sideways. The number of characters might be relatively large, but the number of DOM elements across will always be minimal.

eddyystop commented 10 years ago

Please check out occlusionTable again. It now does horizontal scrolling and supports pinned columns.

Issues:

Todo:

What do think of names like clippingTable, clippingList?

eddyystop commented 10 years ago

occlusionTable updates:

eddyystop commented 10 years ago

What description do you want to appear for Datatable in README.md?

Satyam commented 10 years ago

Sorry, I'm a little behind in following your scrolling table. I hope my next pull request will excuse my tardiness.

Which readme are you referring to?

Satyam

On 28/06/14 16:04, Eddyystop wrote:

What description do you want to appear for Datatable in README.md?

— Reply to this email directly or view it on GitHub https://github.com/eddyystop/mithril-components/issues/4#issuecomment-47428247.

eddyystop commented 10 years ago

I was referring to the root readme, mithril-components/README.md .

As an aside, I placed an image into /components/occlusionTable and display it in /components/occlusionTable/readme.md. A picture IS worth a 1,000 words.

Satyam commented 10 years ago

You already have:

The latest update added row selection.

I'll keep this readme in mind as I add features.

You might want to add links from the list of components and mixins to the readmes in each of the components in the repository, not just references to the sources of the ideas.

Satyam

On 28/06/14 17:10, Eddyystop wrote:

I was referring to the root readme, mithril-components/README.md .

As an aside, I placed an image into /components/occlusionTable and display it in /components/occlusionTable/readme.md. A picture IS worth a 1,000 words.

— Reply to this email directly or view it on GitHub https://github.com/eddyystop/mithril-components/issues/4#issuecomment-47429784.

Satyam commented 10 years ago

On 28/06/14 16:52, Satyam wrote:

Sorry, I'm a little behind in following your scrolling table. I hope my next pull request will excuse my tardiness. This is what I was doing: https://github.com/Satyam/mithril-components/tree/bsform/components/bsform

The BS abbreviation comes from Bootstrap but it may carry its usual meaning ;-) .

It already has an HTML to test it.

I'll do the pull request when I have more of the TODOs done.

Satyam

eddyystop commented 10 years ago

I've considered what we basically could do is port the solutions of a well-considered library of "components" to Mithril, rather than build components without a roadmap. We don't have to insist on a semi-compatible interface, just have Mithril components 'inspired' by another library.

There are multiple target libraries, some popular like BS, others not hugely popular but well conceived such as the belated Enyo. There are older libraries targeted to desktops, others to mobile.

Any thoughts?

eddyystop commented 10 years ago

Do you consider clippingTable to be an improvemnt over occlusionTable?

eddyystop commented 10 years ago

I will make an example, if you don't mind, that combines component/bsform and mixin/FormMixin. I can use mixin/Solder or not.

eddyystop commented 10 years ago

I've pushed an example using BootstrapForm and FormMixin as public/bsform2.html. You can start the server in the repo with node app.js to see the POST and response.

It may make sense to separate the building of forms from the request. Feel free to change FormMixin.

Please see if chriso/validator.js (used by ValidationMixin) suits BSForm's validation needs. If you decide to use another library, I'll look into converting ValidationMixin to that as it's better this repo settle on one validation library. Of course feel free to change ValidationMixin.

Satyam commented 10 years ago

You did a great job with the tables. The one with pinned columns is great. Developers often ask for that feature. As always, when you use two tables, it is hard to get the columns or rows aligned. For the table with pinned columns, the only alternative to allow multi-row cell contents is to check the height of the corresponding rows on each table and explicitly enlarge the shorter one.

A general comment on components. It is better not to add too many arguments, specially optional arguments. In the table view, if you want to set an attribute, you have to pass undefined on the previous arguments if you don't care to set them. It is better to have the view receive a controller instance and an object with configuration attributes. It is easier to remember attributes by name than by ordinal position within the argument list.

Second, leading underscores are not a good idea. The convention used to be that variables starting with underscore were meant to be private while those that started with $ were automatically generated. In datatable, I have an optional classNames configuration attribute which holds all the optional CSS classNames. There is no point in inventing funny names to have a flat configuration object, just nest them under a grouping category.

You might also want to check this post to improve performance. You say that in the demo you render the three tables twice but I am not sure that is completely true. I think that each individual call to m.render() to do the second round of rendering forces the other two tables to render as well. However, I'm not completely sure of this, I believe Mithril tries to avoid too many consecutive refreshes and postpones a new refresh until a certain time has passed so that other refreshes might come in and then all be done at once.

Anyway, that second refresh of yours would still be forcing any other elements in the application page to refresh, unless Mithril holds on to that. The scroll is also triggering refresh request on all of the page. Admittedly, Mithirl won't refresh the DOM if not required, but it certainly runs the view method of everything that is on the page and does the diffing.

You might benefit from the technique described in my post so that refreshes internal to the datatable don't cause refresh requests elsewhere in the page.

Since, in general, a scroll is an internal matter to the table, you might provide a configuration attribute for the developer to set a callback to be notified if the table has scrolled. Unless there is a callback set on this configuration, you may keep the scrolling completely internal. You can see this happening here where I check for a function on the onCellClick configuration attribute. If there is one, then I prepare the info for the callback and prime the refresh prior to calling the callback. Likewise with row selection.

eddyystop commented 10 years ago

I check a re-render-is-scheduled flag and then do a setTimeout. So all the tables are re-rendered only once. I consider this temporary until I make an example of your partial re-render technique (which I can then include as a pattern) and then implement it.

I have row selection with a callback on my todo list.

... What's a use case for having a scroll callback?

... Do you think names like bigTable, bigList are better than clippedTable and occlusionTable?

I totally agree with using an options parameter. I was hesitant because the following may be unsettling to people looking at a component's example. But frankly I have the same thing now, just spread out.

options = {
  height: 250,
  width: '750px',
  selectors: {
    _wrapper: '#table2', _parent: '.parentSelector', _heading: '.headingSelector',
    _tr: '.trSelector', _odd: '.oddSelector', _even: '.evenSelector',
    '4': '.row4Selector'
  },
  attrs: {
    _wrapper: {wrapperAttr: ''},  _parent: {parentAttr: ''},
    _heading: {style:{
      backgroundColor:'Aqua', 
      height: '40px'
    }},
    _tr: {trAttr: ''},  _odd: {style:{backgroundColor:'LightGreen'}},  _even: {style:{backgroundColor:'Khaki'}},
    '4': {style:{
      backgroundColor:'Red'
    }}
  }
};

... So our fcn signatures start with a few mandatory params, and then one options param?

The leading underscore was a mistake. I wanted to prevent naming conflicts within m(,,attr) with attr key values the dev may be using. Anyway to shorten this topic I'll switch '_wrapper" to 'wrapper'.

... I still like selector (a Mithril term) over className as the latter doesn't suggest m('button[type=button]') is allowed.

eddyystop commented 10 years ago

I've changed all of my controllers, bringing their signatures in line with what we talked about, and improving them. They have readme.md in markup. The root README has links to their code, running them and images of the results.

I rewrote the tab component completely. Its now more flexible and easier to understand. The readme has a pic showing the results of a nice example.

This tab component is partially inspired by Boootstrap. Since tabs are a nav Bootstrap thing rather than a form or layout thing, I was thinking of making it even more Bootstrapy. If you don't mind of course.

Could you do the following for Datatable?

The links for Datatable in the root README are already there.

Satyam commented 10 years ago

Sorry for being late. BootstrapForm is requiring more effort than I had envisioned. Regarding "... What's a use case for having a scroll callback?", no reason I can think of, it was a bad example of the general rule: if the developer doesn't care to be notified about internal changes within the component, no changes to elements outside of the component itself should be expected.

I'll take a break from BSF and do the datatable stuff ASAP.

Satyam commented 10 years ago

I did the changes but I messed up in trying to fix the links in the main readme.md file so I won't do the pull request for that one. The changes are simple enough:

Current:

[(Sample usage.)](/public/datatable.html)
[(Results.)](/components/datatable/sample.png)

Should be:

 [(Sample usage.)](/public/Datatable.html)
 [(Results.)](/public/images/datatable.png)

I preferred to locate the image not along the component but in the /public along the sample.

TomKaltz commented 10 years ago

Re: occlusionTable

@Satyam: It does look good! I really don't remember why there was such a fuzz about making the table scroll smoothly. Scrolling a full record at a time looks pretty good. After all, the mousewheel does not roll smoothly, it is jumpy on purpose.

The technique is very performant but the experience varies from browser to browser.

Tested the following page... http://eddyystop.github.io/mithril-components/public/occlusionTable.html ...on the following browsers on Mac OS 10.10.

Safari 8.0 (10600.1.25): Lots of flickering during mousewheel scroll. Even more flickering, and vertical jittering of whole table element while scrolling with trackpad.

Firefox 31.0: Less flickering during mousewheel and trackpad scroll. Same symptom of vertical jittering while scrolling with either.

Chrome 38.0.2125.111: Performs the best, no vertical jitter, flicker seems to only occur very very seldomly.

Also, Mobile Safari and Chrome, all problems persist and make the table very clunky-looking, negating the performance gain IMHO.

I'm wondering if a more typical virtual scrolling method is able to be implemented in Mithril. Perhaps having a set amount of virtual DOM elements that are moved from top-to-bottom as they scrool off the screen (and vice versa, if scrolling upward). And then using a single blank div on the top and bottom of visible elements that adjusts in regards to the scroll position. Any thoughts?

eddyystop commented 10 years ago

If memory serves me well ... the flickering can be improved if not eliminated. The above code checks the height of the first row and assumes each row is the same height. It prioritizes rendering only the rows which can be viewed over a smooth scroll. (A disease called occlusionitis?)

I believe you would get a smoother experience if, first, the code assumed each row was at least a minimal height, say 15px, and thus accept rendering a few more rows than can be viewed. Second, play around with the vertical offsets of the < div> within the scroll area to produce the smooth scroll.

Lastly, this code uses a fat controller. We've since learned thin ones are better.