Skip to content

Conversation

@h-kanazawa
Copy link
Contributor

@h-kanazawa h-kanazawa commented Aug 16, 2019

Which issue does this close?

Closes

Describe your changes providing screenshots of UI changes if necessary.

  • Enable show stats in SourceResults
  • Viz Refactor custom template PIE, COUNTER, XVSY, and FREQUENCY

@h-kanazawa h-kanazawa force-pushed the LL-151-add-stats-to-tables branch 3 times, most recently from b5901e6 to efe0b2f Compare August 23, 2019 13:18
@h-kanazawa h-kanazawa changed the title WIP: LL-151 Add stats to SourceResult tables LL-151 Add stats to SourceResult tables Aug 23, 2019
Copy link
Contributor

@lydiamross lydiamross left a comment

Choose a reason for hiding this comment

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

Looks good!

The ticket says that the totals should be fixed when scrolling to keep them visible but the rest of the rows would disappear as you scroll through the table. Does this not work in reality?

@h-kanazawa
Copy link
Contributor Author

The ticket says that the totals should be fixed when scrolling to keep them visible but the rest of the rows would disappear as you scroll through the table. Does this not work in reality?

I discussed with @ryansmith94, fixing totals became out of scope of this PR because the design and UX issue. However ScrollableTable component has the options to fix header and footer. Fixing is easy work.

@h-kanazawa h-kanazawa force-pushed the LL-151-add-stats-to-tables branch from efe0b2f to 964d503 Compare September 3, 2019 11:19
Copy link
Member

@ryasmi ryasmi left a comment

Choose a reason for hiding this comment

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

Nice Hota. Refactor here looks good too.

Ian247
Ian247 previously requested changes Sep 5, 2019
Copy link

@Ian247 Ian247 left a comment

Choose a reason for hiding this comment

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

Capitalisation of text is not consistent. Top two toggles both have capitals, bottom toggle does not.
image

Totals do not stick on table, so when scrolling on either top or bottom selected the values disappear. The table should scroll and the totals remain in place regardless of which option you choose.

@lydiamross
Copy link
Contributor

@Ian247 see Hota's reply a couple of days ago about the scrolling:

I discussed with @ryansmith94, fixing totals became out of scope of this PR because the design and UX issue. However ScrollableTable component has the options to fix header and footer. Fixing is easy work.

@ryasmi
Copy link
Member

ryasmi commented Sep 6, 2019

@lydiamross good spot, don't worry though. Discussed it with Ian and decided that now we don't offer the option to display top and bottom at the same time they can stick. Hota originally implemented this with the stickiness but when top and bottom were shown together you didn't get to see any data rows because there wasn't enough room.

@ryasmi ryasmi changed the title LL-151 Add stats to SourceResult tables feat(Visualisations): Adds options to display rows for min, max, average, and total for visualisations shown as tables. (LL-151) Sep 6, 2019
@h-kanazawa
Copy link
Contributor Author

@Ian247 I capitalised the label "bottom / top". As we discussed, sticking table head/foot is still OFF.

@ryasmi
Copy link
Member

ryasmi commented Sep 6, 2019

Thanks @h-kanazawa please add the stickiness before this is code reviewed again (see my earlier comment about why stickiness is ok now)

@h-kanazawa
Copy link
Contributor Author

@ryansmith94 Thanks. I read your comment.
This morning, I discussed stickiness with Ian and Ben. Our decision is no stickiness at the moment because sticked tables collapse the design when dashboard widget height is short.

@ryasmi
Copy link
Member

ryasmi commented Sep 6, 2019

Ah okay cool, thanks @h-kanazawa 👍

@ryasmi ryasmi merged commit ac88386 into master Sep 10, 2019
@ryasmi ryasmi deleted the LL-151-add-stats-to-tables branch September 10, 2019 09:24
@HT2Bot
Copy link
Member

HT2Bot commented Sep 10, 2019

🎉 This PR is included in version 4.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants