Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternative PivotUI2 function #1046

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bseddon
Copy link

@bseddon bseddon commented Nov 9, 2018

The PivotUI function generates a presentation layout based on a table. The challenge presented by using a table is that if a different presentation of the high level components (renderer, aggregator, report area, page, column and row fields) is required the use of the <table> tag is too prescriptive.

The alternative function, PivotUI2, instead just generates a set of <div> tags. These can then be presented in any way that is required by an application. For example, in a mobile app it may be more helpful to stack all the high level components on top of each other. In a responsive page it be be useful to be able to switch between a stacked presentation for narrow pages and a conventional layout for normal pages.

The pivot.css file has been updated to provide an example, default, styling that is a facsimile of the table layout. It's not an exact copy, though it could be with a little more effort. The sample styling is provided to show that the use of <div> tags does not preclude the existing table layout. The table layout is achieved using display:grid and the related grid styling directives.

PivotUI generates HTML using a table which has a fixed layout.  The alternative UI generates a set of <div> tags.  The presentation of these tags can be defined using only CSS. The updated ./dist/pivot.css includes CSS that create a facsimile of the table presentation.  The facsimile is not exactly the same but it can be with a little more effort.
The styles create a presentation that is a facsimile of the PivotUI table based layout.  The facsimile is similar to the table layout but not identical - but it could be with a bit more effort,  It is provided to illustrate how the generated <div> tags can be manipulated.
@nicolaskruchten
Copy link
Owner

This is very interesting, thank you! Nice work!

I don't love the idea of maintaining two functions like this, but I do like the idea of getting away from <table> tags so perhaps instead of making this pivotUI2 we could just make this a breaking change by bumping the version number of PivotTable.js to 3.0 and make this the primary pivotUI function. I'll give it some serious thought in the days to come.

Version 0.5.5 references a version of require-dir that uses invalid node because some properties were deprecated in node 8.x.  gulp-git version 2.8.0 references require-dir 1.0.0 which doesn't use the invalid property.
It is required to prevent a gulp crash with the message:

'''gulp[16200]: src\node_contextify.cc:628: Assertion `args[1]->IsString()' failed.```
@nicolaskruchten
Copy link
Owner

As a different approach to supporting both functions, could we imagine this implemented as a series of if statements controlled by a new parameter, rather than duplicating quite so much code?

@bseddon
Copy link
Author

bseddon commented Nov 11, 2018

Definitely. I implemented the alternative function separately because otherwise I'd hacking existing code but I agree that using a boolean option and a selection of conditions will reduce the duplicated code. How about 'responsiveLayout'?

Would you like me to implement an initial pass?

@nicolaskruchten
Copy link
Owner

I think I'd call the option tag, defaulting to table but also accepting div, rather than responsiveLayout, as the latter makes a bit of a promise about responsiveness that I'd rather not commit to ;)

If you feel like implementing this, it would definitely be something I'd consider merging in quite quickly, and I might even port it to react-pivottable as it seems like a solid addition!

Now there is one UI function.  The layout HTML generated can be <table> based or <div> based depending upon the value of a new option called 'tag'.  This option can have the value of 'table' (the default) or 'div'.
@bseddon
Copy link
Author

bseddon commented Nov 12, 2018

I have updated the PR to emit a

or
based layout depending upon value of a new option called 'tag' as you suggest. The tag option can take the value of 'table' or 'div'. It defaults to 'table' if not explicitly defined or if the value is something else.

Couple of things. There are two lines (958, 959) that I don't know how to exercise so I have not touched them. With your greater knowledge maybe you can suggest a test.

The code to implement the change uses if..else.. clauses exclusively. The code changes could be more compact if it were possible to use the ternary operator. However, probably because I have no experience with CoffeeScript, if I try to use an expression like:

$( opts.tag == 'div' ? '<div>' : '<td>' )

Gulp generates some strange and invalid JavaScript. I would expect this expression means that the string '

' will be passed to the jQuery function if the value of opts.tag is 'div' but the generated code does something else.

Finally, tomorrow I will update the CSS to show how the

based layout can be responsive based on
the width of the page being rendered and to make the table facsimile look more like the table layout.

@nicolaskruchten
Copy link
Owner

Thanks! I'll take a look soon :)

The coffeescript equivalent of (a ? b : c) is (if a then b else c): https://coffeescript.org/#try:(if%20a%20then%20b%20else%20c)

@nicolaskruchten
Copy link
Owner

Lines 958 and 959 have to do with vertical vs horizontal layout of unused attributes. By default, the renderer controls are in the first td of the first tr and the unused attributes in the second td. In 'vertical' mode, they are in the first column instead of the first row.

@bseddon
Copy link
Author

bseddon commented Nov 12, 2018

Armed with at least some CoffeeScript knowledge I'll update the PR to use it's in-line if form of ternary. I'll try to create a view in vertical mode so I can see this this in action.

Further modified to use inline if then ... else ... statements which has the effect of slimming the changes made.
@nicolaskruchten
Copy link
Owner

Nice! I wonder if it wouldn’t be easier to read to define near the top a few variables like tableTag, rowTag and cellTag which would be either table/tr/td or div/div/div and then just use those inside the $() instead of ternaries?

@bseddon
Copy link
Author

bseddon commented Nov 12, 2018

Maybe, if it's OK I'll leave that to you. I'm just adding an update to the CSS to handle responsiveness - the reason for suggesting the change (and making it separate) is that I need a responsive layout to the pivot table components.

Added support for a responsive layout but adding the 'responsive' tag which takes values true or false.  This option only had an effect when the 'tag' option is set to 'div'
Updated the CSS for the <div> layout to more closely resemble the table layout.  Also added directives to illustrate how the <div> layout can be responsive.  The directive change the layout to a stacked presentation when the display device is a handheld one such as a mobile phone or if the page width is 800px or less.
@bseddon
Copy link
Author

bseddon commented Nov 12, 2018

I have updated the PR with two further changes. One change is to pivot.coffee to add a 'responsive' option. This is only applicable when the 'tag' option value is 'div'. The effect is to add a class 'pvtUIResponsive' alongside 'pvtUi' to the main <div> element (the one that would be <table>) when the 'responsive' value is true.

The purpose of this change is to allow the layout to be able respond to directives added to the pivot.css file. Before I add some comments about this, the previous directives I added have been updated so the <div> layout more closely resembles the <table> layout.

The new directives added take advantage of the possible existence of the 'pvtResponsive' class. Now when 'tag' is 'div' and 'responsive' is true and the page width is greater than 800px the layout assumes the <table> layout. When the page width narrows to 800px or less or the display is on a hand held device such as a mobile phone, the layout automatically reconfigures to a stacked layout. The selection of 800px is arbitrary and is defined by the CSS @media directives so users can change or override the transition width or create other transition points of their own. The choice of a stacked layout is also arbitrary but, again, a user is able to provide their own CSS to create their own preferred responsive presentation if the default one does not work for them.

As I write this, it occurs to me that instead of adding a 'responsive' option it may be better to add an option called something like 'customClass' so the user can provide an additional class that is added to the main <div> element. One of the possible values for the class would be 'pvtResponsive'. This approach will not appear to explicitly offer a 'responsive' mechanism as I know from an earlier reply you are uncomfortable with this suggestion of a promise.

@Shoti2313
Copy link

will this be in v3 soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants