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

Pulse conditional table formatting #8032

Merged
merged 18 commits into from Jul 25, 2018

Conversation

@senior
Copy link
Contributor

commented Jul 11, 2018

This creates a new shared javascript file in
resources/frontend_shared that will be used by the frontend and the
backend to drive the background color of cells using conditional
formatting features the frontend has.

This uses the JDK 8 included Nashorn javascript engine to eval and
invoke the functions in that shared file.

TODO

  • Get the real javascript color choosing script added
    • Update the smoke test
    • Manual testing and visual comparison
    • Check runtime impacts of using the javascript engine

senior and others added some commits Jul 11, 2018

Driver pulse table cell color from visualiation settings
This creates a new shared javascript file in
`resources/frontend_shared` that will be used by the frontend and the
backend to drive the background color of cells using conditional
formatting features the frontend has.

This uses the JDK 8 included Nashorn javascript engine to eval and
invoke the functions in that shared file.
Switch from `make-array` to `object-array`
More concise and avoids a lint error.
Convert visualization settings and columns to JS data
Passing in the clojure data structures directly to the Nashorn code
causes Java objects to be used that don't have all of the functions
that regular Javascript objects do, such as `map`. This commit
dumps that data into a JS array/map before invoking the Nashorn code.

@tlrobinson tlrobinson force-pushed the pulse-conditional-formatting branch from 5710b24 to 05b5b51 Jul 17, 2018

@tlrobinson tlrobinson force-pushed the pulse-conditional-formatting branch from 05b5b51 to 8d000dc Jul 18, 2018

@mazameli

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

Looks like this all works. I didn't experience any significant slowness. Main jankiness is the spacing between the columns in the rendered email, which I think I remember @tlrobinson mentioning. Seems like the recipe for bad spacing is when there are two consecutive columns that are left-aligned and with content longer than their column heading.

Also, I'm the one responsible for the monospaced type treatment for Number fields (added in 0.29 or .28 I think), which with fresh eyes looks like it was maybe a Bad Idea™, so Tom if you feel strongly enough about it we could undo that.

spacing

@tlrobinson tlrobinson changed the title WIP - Driver pulse table cell color from visualiation settings Driver pulse table cell color from visualization settings Jul 24, 2018

@tlrobinson tlrobinson changed the title Driver pulse table cell color from visualization settings Pulse conditional table formatting Jul 24, 2018

@salsakran

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

doesn't seem to work in the preview for me

in the QB
screen shot 2018-07-24 at 4 09 24 pm

in the preview + slack

screen shot 2018-07-24 at 4 10 03 pm

for some reason I'm getting errors when I try to send a test email


07-24 16:14:41 DEBUG metabase.middleware :: POST /api/pulse/test 400 (1 ms) (0 DB calls).
{:errors {:cards "value must be an array. Each value must be a map with the keys `id`, `include_csv`, and `include_xls`. The array cannot be empty."}}

@tlrobinson tlrobinson merged commit c962904 into master Jul 25, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@tlrobinson tlrobinson deleted the pulse-conditional-formatting branch Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.