Implement inline styles for FlexColumns

[MikeMcElroy]

Sometimes, it's desirable to mark up a column with inline styles without having to specify a cellRenderer function. eg. Trying to draw attention to a sorted column by giving it a background color, or applying a textTransform to the contents. This PR implements the ability to use inline styles with your column simply and easily. It also conservatively merges in the computed styles related to maxWidth, minWidth, and flex.

[MikeMcElroy]

codecov/changes seems to think I added a bunch of files. Not sure what's up there.

[bvaughn]

Thanks for putting this PR together. I'm currently chatting with @kof on issue #197 about a related topic so I'd like to hold off on this until I've decided how to proceed there. (The themes we're talking about might also support styles so...) Feel free to check out that thread (and the related one here) if you'd like to add any input.

[kof]

It is still important to differentiate themes and runtime styles, if this is something a user will want to modify at render time, then it should be part of style object in props.

[MikeMcElroy]

@kof I just finished looking over the discussions @bvaughn linked to, and I wholeheartedly agree. Being locked into a particular theming style or structure of style management by an external component would be a hearty 👎.
(ETA)
Not to say you couldn't support something like react-theme on top of RV, but at the very least supporting "natural" JSX semantics like className and style would be the most flexible approach.

[MikeMcElroy]

I feel like I totally botched my PR by checking in dist/ code, but it seemed like the right choice at the time (considering I had to push my built code to my branch to test it live in my app). Now that you've updated to 6.3.0, I'm not sure how to resolve the resultant conflict (in a sourcemap file).

My first instinct is to just nuke this PR and create another one that's fully up to date, but I will wait to hear what you'd like to see first.

[kof]

because dist should never be in the repository, it should be published to npm

[MikeMcElroy]

That's what I thought, but the fact that it was in the repo made me second-guess myself. :-)

[bvaughn]

because dist should never be in the repository, it should be published to npm

Binary dist files don't belong in a repository because they can't be diffed and so the storage is inefficient. Whether or not JavaScript dists belong in a repository is a matter of opinion. There are a lot of practical benefits including:

• Easy for a maintainer to verify what's changed between tags.
• Easy for contributors (using rawgit.com) to link Plunkers or other demo files to different versions of a library to demonstrate problems. (Granted this is also possible with npmcdn.)

Anyway, try not to assume such a one-size-fits-all outlook. PRs probably shouldn't include dist as a general rule- but even so, it's very easy to recover a PR like this. Just reset the dist folder to origin/master and re-build NPM if you want to test your updated changes.

[MikeMcElroy]

@bvaughn Thanks for the advice -- I'll give it a shot.

[bvaughn]

To be honest though, I have considered (and still do consider) removing dist from Git since it does get in the way sometimes (like this). It's just not an obvious choice (for me) as it does provide some conveniences. :)

[kof]

it was always a pain for me and lots of other popular projects I watch, so common sense I could recognize was do add dist to .gitignore and to add .src to .npmignore and generate lib and dist, where lib is a transpiled version. Putting es2015 into src will cause you also problems, see for e.g. redux setup.

[bvaughn]

It hasn't caused any problems for me (excluding this PR) and it has been convenient when I want to quickly performance test arbitrary version ranges of the library. I do understand why you prefer that approach though. I just think it should be up to the library maintainer.

I should add that I'm sure it helps that I'm the only maintainer of this project. I can see where you'd want to avoid keeping dist in git for a project with multiple active maintainers.

Putting es2015 into src will cause you also problems, see for e.g. redux setup.

Not sure I understand what you mean?

[bvaughn]

Getting back to the topic of this PR...

It is still important to differentiate themes and runtime styles, if this is something a user will want to modify at render time, then it should be part of style object in props.

I'm not convinced it's practical to support inline style objects for components as complex as some of the react-virtualized ones. It's simple when we're talking about a component like Alert but FlexTable for example has a lot of internal parts. Allowing each one of them to be customized by either a class-name or style param seem like it might get bloated.

[kof]

That is true, it depends on whether a style object can break the component or not. So either leave it up to user and make him responsible or support only specific props, which are then applied to the style if needed.

[bvaughn]

Yeah. My goal has been to keep react-virtualized components as invisible as possible. They set flex properties and such but they avoid things like borders, background colors, etc. As much as possible I prefer for that sort of style to be set by the container (for wrapping styles like background color and border) and by the renderer (for inner styles) because that's the most flexible solution. You can use CSS/JSS/LESS/styles/whatever-you-want.

I think FlexTable is the one exception to this because without a few minimal styles, headers wouldn't make sense.

[bvaughn]

That being said, the type of change made in this PR would already be supported by specifying an inline style via the column renderer. It's just a question of convenience.

@MikeMcElroy I wonder if you considered using a HOC-like-approach for this? Maybe something like...

export function styledCellRenderer (style: Object): Function {
  return function renderer (
    cellData: any,
    cellDataKey: string,
    rowData: any,
    rowIndex: number,
    columnData: any
  ): string {
    const displayText = cellData == null
      ? ''
      : String(cellData)

    return (
      <div style={style}>
        {displayText}
      </div>
    )
  }
}

Then you could reuse it in a more lightweight way...

<FlexColumn
  cellRenderer={
    styledCellRenderer({
      backgroundColor: 'blue'
    })
  }
  displayKey='id'
  header='ID'
  width={50}
/>

[MikeMcElroy]

@bvaughn I actually did try that, and got this output:

when what I wanted was this:

[bvaughn]

@MikeMcElroy Your style probably just needs to specify a height: 100% value, no?

[MikeMcElroy]

For one, I just tried that, and it doesn't work because the output of cellRenderer winds up in the truncated text div, which does not have height:100% specified. For two, it's still not as flexible as using an inline style on the column -- if I want a background color on already custom-rendered content, I'll have to wrap my cellRenderer in the styledCellRenderer, and remember to set the height to 100%. It seems like that makes it needlessly difficult. There are many ways to accomplish a task in JS/HTML/CSS, but certain ways require more complexity than others.

[MikeMcElroy]

Also, you do allow the user to directly style the containing div through classes (cellClassName is appended to the "outer cell div"'s className attribute. I'm hoping that we can allow for style semantics to behave this same way. Perhaps style isn't the right name for it, but cellStyle, or cellContainerStyle?

[bvaughn]

@MikeMcElroy~ Just a friendly reminder that I'm just one guy and I maintain react-virtualized and my other OSS projects in my spare time. So when feature requests come in, before committing to do them, I need to consider a few things.

• Are there any available workarounds available to users that wouldn't be an unreasonable amount of effort for them and would prevent requiring a library update? Many times this is the case. That's why I asked if you had considered something like what I wrote above.
• Is it something I'll have the time to manage going forward? Each new feature adds complexity and potential for bugs. It's great when someone submits an initial PR to add a feature- but even so I'm probably the one who will end up maintaining it going forward.
• Will it negatively impact performance?
• Does it fit with the rest of the interface in a cohesive way or is it just a one-off for a very specific use case?

I think we probably both want the same thing here- a better, more flexible interface for users. In my thinking, the styling/theming changes that @kof and I have been discussing will probably be released with version 7. If I'm going to override the styling system, I might like to remove some of the redundant class-name properties (so I don't end up supporting multiple ways to do the same thing) and that will require a major release. So it's probably something that won't happen immediately as I'll need to give it a lot of thought beforehand. (If you're interested, I have a version 7 roadmap wiki page here.) So I mentioned what I said above as a potential, temporary workaround for you.

[MikeMcElroy]

@bvaughn By no means was my argument meant as a slight at your awesome work w/ RV, and I apologize for it seeming that way. I'm currently in a POC stage with RV at work, and the inline style battle has been fought already and decided on internally, but if I can't reasonably show that we can style columnally (inventing words FTW), it's a major ding against RV for our uses.

I tend to think of the changes you're talking about with @kof as being orthogonal to this concern, but if you need to think more about it, I respect that.

[bvaughn]

No worries. Text communication is difficult. It's often hard to tell what someone means. I appreciate the added insight into what's happening on your end.

The use case you're describing is actually 2 features rolled into 1:

• Support custom inline styles.
• Support styling an entire FlexTable column (not just individual cells).

Even if there were inline style equivalents to all of the currently-supported class properties, you still wouldn't be able to style the column in the way you're trying to (without overriding the default height of .FlexTable__truncatedColumnText). I agree this is too much hassle. FWIW deciding on a default DOM/style structure for FlexTable cells was obnoxious. I think most people want text to truncate inside of a "table" cell but the flex container spec doesn't make that easy. That's why I had to add the nested <div>.

Hm. Maybe I can put out a quick point release that adds some kind of column-style-support. Let me think on it a bit more.

[MikeMcElroy]

Ping on this issue re: v7?

[bvaughn]

Still working on v7 (see PR #206). Hit a bit of a snag with dynamic cell measuring that I'm trying to work through. Other than that, I think styling/theming is the last big area to be finished.

Working on it as fast as I can but... I'm sick this week and I have my day job, so it's slow going.

[MikeMcElroy]

Just looking for an update. Carry on.

[bvaughn]

No worries. Haven't forgotten. Just been a bit overwhelmed this week. :)

[bvaughn]

There's a note on PR #206 about something that's causing problems with the CellMeasurer:

Figure out why CellMeasurer does not play nicely with VirtualScroll. (It's currently throwing the error, "Only a ReactOwner can have refs.".)

If you find yourself feeling ambitious and charitable, feel free to check it out. :)

[bvaughn]

Closing in favor of version 7 PR #206 (which I plan to merge momentarily). It supports a style property on FlexColumn as you've proposed. :)