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

Add option to hide Pivot Table totals #3943

Merged
merged 4 commits into from
Jun 30, 2019
Merged

Add option to hide Pivot Table totals #3943

merged 4 commits into from
Jun 30, 2019

Conversation

arikfr
Copy link
Member

@arikfr arikfr commented Jun 30, 2019

What type of PR is this? (check all applicable)

  • Feature

Description

Added toggles to hide/show the totals row/column in a pivot table.

Related Tickets & Documents

https://discuss.redash.io/t/cohort-heatmap-displayed-incorrectly/4075/6

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Copy link
Collaborator

@kravets-levko kravets-levko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, just a single note 👍

client/app/visualizations/pivot/Editor.jsx Outdated Show resolved Hide resolved
@ranbena
Copy link
Contributor

ranbena commented Jun 30, 2019

From a UX stand point, since the switch element has a connotation of "on means more, off means less", I would consider flipping the bool - "Show Pivot controls" instead of "Hide".

@ranbena
Copy link
Contributor

ranbena commented Jun 30, 2019

Also, give some spacing between the vis name text field and the switches.

@kravets-levko
Copy link
Collaborator

@ranbena Maybe, change "Hide pivot controls" as well? 🤔

@ranbena
Copy link
Contributor

ranbena commented Jun 30, 2019

@ranbena Maybe, change "Hide pivot controls" as well? 🤔

Not sure what you mean - that's what I suggested, no? 🤔

@arikfr
Copy link
Member Author

arikfr commented Jun 30, 2019

From a UX stand point, since the switch element has a connotation of "on means more, off means less", I would consider flipping the boo

I did just that when implementing @kravets-levko feedback.

I did leave the old switch as is, but will see if it's not too much effort to change it as well.

@kravets-levko
Copy link
Collaborator

Not sure what you mean - that's what I suggested, no?

For some reason I decided that you mean only two new options 😅

@arikfr
Copy link
Member Author

arikfr commented Jun 30, 2019

Flipped them all: d602f0d.

image

Copy link
Collaborator

@kravets-levko kravets-levko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, works fine 🚀

Copy link
Contributor

@ranbena ranbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arikfr
Copy link
Member Author

arikfr commented Jun 30, 2019

Give it a top margin

Given. Although it something that we should probably handle at the container component instead of each editor on its own.

@ranbena
Copy link
Contributor

ranbena commented Jun 30, 2019

Give it a top margin

Given. Although it something that we should probably handle at the container component instead of each editor on its own.

Considered it. But most of the visualization editors have tabs which I'd rather not affect.

@arikfr arikfr merged commit 6748e9a into master Jun 30, 2019
@arikfr arikfr deleted the pivot-totals branch June 30, 2019 12:43
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
* Add option to hide Pivot Table totals

* Simplify implementation using DEFAULT_OPTIONS.

* Flip hide pivot controls to show pivot controls

* Update client/app/visualizations/pivot/Editor.jsx

Co-Authored-By: Ran Byron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants