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.

Sign up for GitHub

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

Jump to bottom

Virtualize AccordianList to resolve performance issues with many dbs/schemas/tables/fields #6324

Merged
merged 20 commits into from
Dec 6, 2017
Merged

Virtualize AccordianList to resolve performance issues with many dbs/schemas/tables/fields #6324

merged 20 commits into from
Dec 6, 2017

Conversation

tlrobinson
Copy link
Contributor

@tlrobinson tlrobinson commented Nov 3, 2017 •
edited
Loading

TODO
  • fix layout issues
    • missing divider line between last item of expanded section and header of next section
    • padding at beginning/end of section items
  • integrate CellMeasurer into AccordianList to handle overflowing item/section titles
  • fix automatic height and popover "constrain to screen" behavior
  • Ensure maxHeight is propagated to all instances of <AccordianList>
    • DataSelector
    • FieldList nested directly inside Popover:
      • BreakoutWidget
      • FieldWidget
    • BreakoutPopover
    • AggregationPopover
    • FilterPopover

@tlrobinson tlrobinson requested review from kdoh and attekei and removed request for kdoh November 14, 2017 14:27
@tlrobinson
Copy link
Contributor Author

@metabase/core-developers This is almost ready, and it's worth giving a try now.

The only remaining issue I'm aware of is that because AccordianList now needs to be provided an explicit height (react-virtualize requires it) Popover's sizeToFit feature doesn't work. It's kind of a catch 22 because we don't know how much height there is until we pick the Popover attachment direction, which we don't do until we know the height of the popover content...

@tlrobinson
Copy link
Contributor Author

@metabase/core-developers Alright, I've fixed the remaining known issues. Please review.

@salsakran
Copy link
Contributor

I'm getting a weird height for the popover that is larger than the window height.
screen shot 2017-11-17 at 5 18 26 pm

When I expand the window, it expands the popover so it's still larger than the window height
screen shot 2017-11-17 at 5 17 57 pm

@tlrobinson
Copy link
Contributor Author

@salsakran Ah yes, I forgot to enable the maxHeight functionality on some of the other popovers. Now as long as AccordianList or FieldList is nested directly inside Popover it will work automatically, and I think I've manually handled passing maxHeight in the rest of the cases.

@salsakran
Copy link
Contributor

seems to work pretty well for me!

kdoh
kdoh requested changes Nov 21, 2017
View reviewed changes
Copy link
Member

@kdoh kdoh left a comment

Choose a reason for hiding this comment

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

Couple tiny code things we should probably clean up before merging but overall seems reasonable. Dig the addition of examples. 👍

I'd also really love it if we could get at least a couple unit tests onto AccordianList since there's some pretty intense logic going on in there.

frontend/src/metabase/components/AccordianList.jsx Outdated
const element = this.refs.selected && ReactDOM.findDOMNode(this.refs.selected);
if (element && element.scrollIntoView && !elementIsInView(element)) {
element.scrollIntoView();
this._forceUpdateList();
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have a note here about why this is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

frontend/src/metabase/components/AccordianList.jsx Outdated
@@ -164,91 +198,151 @@ export default class AccordianList extends Component {
return this.props.getItemClasses && this.props.getItemClasses(item, itemIndex);
}

renderSectionHeader(section, sectionIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

Would probably be good to remove this if it's not called anywhere (doesn't look like it is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tlrobinson
Copy link
Contributor Author

@kdoh Thanks, made those changes and added some unit tests.

@metabase/core-developers Anyone else want to review the code?

@salsakran salsakran merged commit a510d8f into master Dec 6, 2017
@salsakran salsakran deleted the virtualized-accordian-list branch December 6, 2017 14:14
@salsakran salsakran added this to the 0.28 milestone Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdoh kdoh kdoh approved these changes

@attekei attekei Awaiting requested review from attekei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.28
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tlrobinson @salsakran @kdoh