-
Notifications
You must be signed in to change notification settings - Fork 45
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
PXD-1291 Make Arranger filters look like our spec #325
Conversation
abgeorge7
commented
Jul 31, 2018
•
edited
Loading
edited
- Modified the Arranger component to divide sections into tabs
- If tab configuration is provided in parameters.js, then it will be divided into tabs. Otherwise it will just be normal Arranger aggregation filters that look more our style.
- Overrode Arranger styles
…a-portal into feat/arranger-portal-on-page
@@ -0,0 +1,77 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same file as in my last PR, just moved to a new folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get the fields name and title from the arranger backend? not hard code in the UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions around dealing with default behavior when tab properties are not explicitly configured ...
<h4 className="data-explorer__filters-title">Filters</h4> | ||
<AggregationTabs | ||
filterConfig={this.props.arrangerConfig.filters} | ||
{...this.props} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if arrangerConfig
is not defined or just {}
?
src/Arranger/AggregationTabs.jsx
Outdated
const tabs = []; | ||
filterConfig.tabs.forEach((tab, i) => { | ||
const tabAggs = aggs.filter(agg => tab.fields.includes(agg.field)); | ||
tabs.push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be aggs.find
instead of aggs.filter
to avoid case where same field appears in more than one tab by accident?
src/Arranger/AggregationTabs.jsx
Outdated
} | ||
/>, | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to a field that is configured in arranger, but not registered in a tab
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would get left out I think - I will write a test for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 👍