You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/11/08 17:08:18 UTC
[GitHub] [superset] geido opened a new pull request, #22064: feat: Horizontal filter bar states
geido opened a new pull request, #22064:
URL: https://github.com/apache/superset/pull/22064
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
geido commented on PR #22064:
URL: https://github.com/apache/superset/pull/22064#issuecomment-1316990111
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on a diff in pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #22064:
URL: https://github.com/apache/superset/pull/22064#discussion_r1021459057
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+ background: ${theme.colors.grayscale.light5};
+ box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+ ${({ theme }) => `
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ justify-content: flex-start;
+ padding: 0 ${theme.gridUnit * 2}px;
+
+ .loading {
+ margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+ padding: 0;
+ }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+ ${({ theme }) => `
+ margin: 0 ${theme.gridUnit * 2}px;
Review Comment:
> I think according to the designs, distance between the gear and this message should be 18px.
Increased to 16px which seems to be the most common spacing value in the horizontal filter bar. I think it looks balanced this way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22064:
URL: https://github.com/apache/superset/pull/22064#issuecomment-1319407921
@jinghua-qa Ephemeral environment spinning up at http://54.185.42.234:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22064:
URL: https://github.com/apache/superset/pull/22064#issuecomment-1316992560
@geido Ephemeral environment spinning up at http://18.236.65.179:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #22064:
URL: https://github.com/apache/superset/pull/22064#issuecomment-1319405142
/testenv up FEATURE_HORIZONTAL_FILTER_BAR=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22064:
URL: https://github.com/apache/superset/pull/22064#issuecomment-1308915304
# [Codecov](https://codecov.io/gh/apache/superset/pull/22064?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22064](https://codecov.io/gh/apache/superset/pull/22064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (62019e2) into [master](https://codecov.io/gh/apache/superset/commit/4e33235020b8bd64dd8022515148da1b3b82c923?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e33235) will **decrease** coverage by `11.52%`.
> The diff coverage is `64.10%`.
```diff
@@ Coverage Diff @@
## master #22064 +/- ##
===========================================
- Coverage 67.03% 55.50% -11.53%
===========================================
Files 1813 1813
Lines 69437 69439 +2
Branches 7450 7450
===========================================
- Hits 46545 38545 -8000
- Misses 20972 28974 +8002
Partials 1920 1920
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.74% <40.90%> (-0.04%)` | :arrow_down: |
| python | `57.66% <40.90%> (-23.88%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.18% <22.72%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
| [.../legacy-plugin-chart-histogram/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhpc3RvZ3JhbS9zcmMvY29udHJvbFBhbmVsLnRz) | `40.00% <0.00%> (ø)` | |
| [...perset-frontend/src/addSlice/AddSliceContainer.tsx](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLnRzeA==) | `59.61% <ø> (ø)` | |
| [...nd/src/components/DropdownSelectableIcon/index.tsx](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRHJvcGRvd25TZWxlY3RhYmxlSWNvbi9pbmRleC50c3g=) | `100.00% <ø> (ø)` | |
| [...t-frontend/src/dashboard/actions/dashboardState.js](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2Rhc2hib2FyZFN0YXRlLmpz) | `37.09% <ø> (ø)` | |
| [superset-frontend/src/dashboard/actions/hydrate.js](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2h5ZHJhdGUuanM=) | `1.78% <ø> (ø)` | |
| [...d/components/DashboardBuilder/DashboardBuilder.tsx](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZEJ1aWxkZXIvRGFzaGJvYXJkQnVpbGRlci50c3g=) | `75.24% <ø> (ø)` | |
| [...ilters/FilterBar/FilterConfigurationLink/index.tsx](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbmZpZ3VyYXRpb25MaW5rL2luZGV4LnRzeA==) | `100.00% <ø> (ø)` | |
| [...Filters/FilterBar/FilterControls/FilterControl.tsx](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2wudHN4) | `29.03% <ø> (ø)` | |
| ... and [343 more](https://codecov.io/gh/apache/superset/pull/22064/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22064:
URL: https://github.com/apache/superset/pull/22064#issuecomment-1319922236
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #22064:
URL: https://github.com/apache/superset/pull/22064#discussion_r1021577750
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
+ await renderWrapper({
+ filterValues: [
+ {
+ id: 'test',
+ type: NativeFilterType.NATIVE_FILTER,
+ },
+ ],
+ });
+ expect(
+ screen.queryByText('No filters are currently added to this dashboard.'),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the empty message', async () => {
+ await renderWrapper();
+ expect(
+ screen.getByText('No filters are currently added to this dashboard.'),
+ ).toBeInTheDocument();
+});
+
+it('should render the gear icon', async () => {
+ await renderWrapper();
+ expect(screen.getByRole('img', { name: 'gear' })).toBeInTheDocument();
+});
+
+it('should not render the gear icon', async () => {
+ await renderWrapper({
+ canEdit: false,
+ });
+
+ expect(screen.queryByRole('img', { name: 'gear' })).not.toBeInTheDocument();
+});
+
+it('should not render the loading icon', async () => {
+ await renderWrapper();
+ expect(
+ screen.queryByRole('status', { name: 'Loading' }),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the loading icon', async () => {
+ await renderWrapper({
+ isInitialized: false,
+ });
+ expect(screen.getByRole('status', { name: 'Loading' })).toBeInTheDocument();
+});
+
+it('should render Add/Edit Filters', async () => {
Review Comment:
```suggestion
test('should render Add/Edit Filters', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
Review Comment:
```suggestion
test('should render', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
+ await renderWrapper({
+ filterValues: [
+ {
+ id: 'test',
+ type: NativeFilterType.NATIVE_FILTER,
+ },
+ ],
+ });
+ expect(
+ screen.queryByText('No filters are currently added to this dashboard.'),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the empty message', async () => {
+ await renderWrapper();
+ expect(
+ screen.getByText('No filters are currently added to this dashboard.'),
+ ).toBeInTheDocument();
+});
+
+it('should render the gear icon', async () => {
+ await renderWrapper();
+ expect(screen.getByRole('img', { name: 'gear' })).toBeInTheDocument();
+});
+
+it('should not render the gear icon', async () => {
+ await renderWrapper({
+ canEdit: false,
+ });
+
+ expect(screen.queryByRole('img', { name: 'gear' })).not.toBeInTheDocument();
+});
+
+it('should not render the loading icon', async () => {
Review Comment:
```suggestion
test('should not render the loading icon', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
+ await renderWrapper({
+ filterValues: [
+ {
+ id: 'test',
+ type: NativeFilterType.NATIVE_FILTER,
+ },
+ ],
+ });
+ expect(
+ screen.queryByText('No filters are currently added to this dashboard.'),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the empty message', async () => {
+ await renderWrapper();
+ expect(
+ screen.getByText('No filters are currently added to this dashboard.'),
+ ).toBeInTheDocument();
+});
+
+it('should render the gear icon', async () => {
Review Comment:
```suggestion
test('should render the gear icon', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
+ await renderWrapper({
+ filterValues: [
+ {
+ id: 'test',
+ type: NativeFilterType.NATIVE_FILTER,
+ },
+ ],
+ });
+ expect(
+ screen.queryByText('No filters are currently added to this dashboard.'),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the empty message', async () => {
+ await renderWrapper();
+ expect(
+ screen.getByText('No filters are currently added to this dashboard.'),
+ ).toBeInTheDocument();
+});
+
+it('should render the gear icon', async () => {
+ await renderWrapper();
+ expect(screen.getByRole('img', { name: 'gear' })).toBeInTheDocument();
+});
+
+it('should not render the gear icon', async () => {
Review Comment:
```suggestion
test('should not render the gear icon', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
+ await renderWrapper({
+ filterValues: [
+ {
+ id: 'test',
+ type: NativeFilterType.NATIVE_FILTER,
+ },
+ ],
+ });
+ expect(
+ screen.queryByText('No filters are currently added to this dashboard.'),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the empty message', async () => {
Review Comment:
```suggestion
test('should render the empty message', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
Review Comment:
```suggestion
test('should not render the empty message', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
+ await renderWrapper({
+ filterValues: [
+ {
+ id: 'test',
+ type: NativeFilterType.NATIVE_FILTER,
+ },
+ ],
+ });
+ expect(
+ screen.queryByText('No filters are currently added to this dashboard.'),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the empty message', async () => {
+ await renderWrapper();
+ expect(
+ screen.getByText('No filters are currently added to this dashboard.'),
+ ).toBeInTheDocument();
+});
+
+it('should render the gear icon', async () => {
+ await renderWrapper();
+ expect(screen.getByRole('img', { name: 'gear' })).toBeInTheDocument();
+});
+
+it('should not render the gear icon', async () => {
+ await renderWrapper({
+ canEdit: false,
+ });
+
+ expect(screen.queryByRole('img', { name: 'gear' })).not.toBeInTheDocument();
+});
+
+it('should not render the loading icon', async () => {
+ await renderWrapper();
+ expect(
+ screen.queryByRole('status', { name: 'Loading' }),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the loading icon', async () => {
Review Comment:
```suggestion
test('should render the loading icon', async () => {
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+const defaultProps = {
+ actions: null,
+ canEdit: true,
+ dashboardId: 1,
+ dataMaskSelected: {},
+ filterValues: [],
+ isInitialized: true,
+ onSelectionChange: jest.fn(),
+};
+
+const renderWrapper = (overrideProps?: Record<string, any>) =>
+ waitFor(() =>
+ render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
+ useRedux: true,
+ }),
+ );
+
+it('should render', async () => {
+ const { container } = await renderWrapper();
+ expect(container).toBeInTheDocument();
+});
+
+it('should not render the empty message', async () => {
+ await renderWrapper({
+ filterValues: [
+ {
+ id: 'test',
+ type: NativeFilterType.NATIVE_FILTER,
+ },
+ ],
+ });
+ expect(
+ screen.queryByText('No filters are currently added to this dashboard.'),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the empty message', async () => {
+ await renderWrapper();
+ expect(
+ screen.getByText('No filters are currently added to this dashboard.'),
+ ).toBeInTheDocument();
+});
+
+it('should render the gear icon', async () => {
+ await renderWrapper();
+ expect(screen.getByRole('img', { name: 'gear' })).toBeInTheDocument();
+});
+
+it('should not render the gear icon', async () => {
+ await renderWrapper({
+ canEdit: false,
+ });
+
+ expect(screen.queryByRole('img', { name: 'gear' })).not.toBeInTheDocument();
+});
+
+it('should not render the loading icon', async () => {
+ await renderWrapper();
+ expect(
+ screen.queryByRole('status', { name: 'Loading' }),
+ ).not.toBeInTheDocument();
+});
+
+it('should render the loading icon', async () => {
+ await renderWrapper({
+ isInitialized: false,
+ });
+ expect(screen.getByRole('status', { name: 'Loading' })).toBeInTheDocument();
+});
+
+it('should render Add/Edit Filters', async () => {
+ await renderWrapper();
+ expect(screen.getByText('Add/Edit Filters')).toBeInTheDocument();
+});
+
+it('should not render Add/Edit Filters', async () => {
Review Comment:
```suggestion
test('should not render Add/Edit Filters', async () => {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #22064:
URL: https://github.com/apache/superset/pull/22064#discussion_r1018310865
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -0,0 +1,298 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/* eslint-disable no-param-reassign */
+import throttle from 'lodash/throttle';
+import React, {
+ useEffect,
+ useState,
+ useCallback,
+ useMemo,
+ useRef,
+ createContext,
+} from 'react';
+import cx from 'classnames';
+import { HandlerFunction, styled, t, isNativeFilter } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import { AntdTabs } from 'src/components';
+import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
+import Loading from 'src/components/Loading';
+import { EmptyStateSmall } from 'src/components/EmptyState';
+import { TabIds, getFilterBarTestId, VerticalBarProps } from './utils';
+import FilterSets from './FilterSets';
+import { useFilterSets } from './state';
+import EditSection from './FilterSets/EditSection';
+import Header from './Header';
+import FilterControls from './FilterControls/FilterControls';
+
+const BarWrapper = styled.div<{ width: number }>`
+ width: ${({ theme }) => theme.gridUnit * 8}px;
+
+ & .ant-tabs-top > .ant-tabs-nav {
+ margin: 0;
+ }
+ &.open {
+ width: ${({ width }) => width}px; // arbitrary...
+ }
+`;
+
+const Bar = styled.div<{ width: number }>`
+ & .ant-typography-edit-content {
+ left: 0;
+ margin-top: 0;
+ width: 100%;
+ }
+ position: absolute;
+ top: 0;
+ left: 0;
+ flex-direction: column;
+ flex-grow: 1;
+ width: ${({ width }) => width}px;
+ background: ${({ theme }) => theme.colors.grayscale.light5};
+ border-right: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ min-height: 100%;
+ display: none;
+ &.open {
+ display: flex;
+ }
+`;
+
+const CollapsedBar = styled.div<{ offset: number }>`
+ position: absolute;
+ top: ${({ offset }) => offset}px;
+ left: 0;
+ height: 100%;
+ width: ${({ theme }) => theme.gridUnit * 8}px;
Review Comment:
Same here
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -0,0 +1,298 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/* eslint-disable no-param-reassign */
+import throttle from 'lodash/throttle';
+import React, {
+ useEffect,
+ useState,
+ useCallback,
+ useMemo,
+ useRef,
+ createContext,
+} from 'react';
+import cx from 'classnames';
+import { HandlerFunction, styled, t, isNativeFilter } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import { AntdTabs } from 'src/components';
+import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
+import Loading from 'src/components/Loading';
+import { EmptyStateSmall } from 'src/components/EmptyState';
+import { TabIds, getFilterBarTestId, VerticalBarProps } from './utils';
+import FilterSets from './FilterSets';
+import { useFilterSets } from './state';
+import EditSection from './FilterSets/EditSection';
+import Header from './Header';
+import FilterControls from './FilterControls/FilterControls';
+
+const BarWrapper = styled.div<{ width: number }>`
+ width: ${({ theme }) => theme.gridUnit * 8}px;
+
+ & .ant-tabs-top > .ant-tabs-nav {
+ margin: 0;
+ }
+ &.open {
+ width: ${({ width }) => width}px; // arbitrary...
+ }
+`;
+
+const Bar = styled.div<{ width: number }>`
+ & .ant-typography-edit-content {
+ left: 0;
+ margin-top: 0;
+ width: 100%;
+ }
+ position: absolute;
+ top: 0;
+ left: 0;
+ flex-direction: column;
+ flex-grow: 1;
+ width: ${({ width }) => width}px;
+ background: ${({ theme }) => theme.colors.grayscale.light5};
Review Comment:
Maybe promote `theme` to the top level to avoid repeating code?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -352,166 +233,56 @@ const FilterBar: React.FC<FiltersBarProps> = ({
});
}, [dataMaskSelected, dispatch, setDataMaskSelected]);
- const openFiltersBar = useCallback(
- () => toggleFiltersBar(true),
- [toggleFiltersBar],
- );
-
- const onScroll = useCallback(
- throttle(() => {
- clearTimeout(timeout.current);
- setIsScrolling(true);
- timeout.current = setTimeout(() => {
- setIsScrolling(false);
- }, 300);
- }, 200),
- [],
- );
-
- useEffect(() => {
- document.onscroll = onScroll;
- return () => {
- document.onscroll = null;
- };
- }, [onScroll]);
-
useFilterUpdates(dataMaskSelected, setDataMaskSelected);
const isApplyDisabled = checkIsApplyDisabled(
dataMaskSelected,
dataMaskApplied,
nativeFilterValues,
);
const isInitialized = useInitialization();
- const tabPaneStyle = useMemo(
- () => ({ overflow: 'auto', height, overscrollBehavior: 'contain' }),
- [height],
- );
- const numberOfFilters = nativeFilterValues.length;
-
- return (
- <FilterBarScrollContext.Provider value={isScrolling}>
- <BarWrapper
- {...getFilterBarTestId()}
- className={cx({ open: filtersOpen })}
- width={width}
- >
- <CollapsedBar
- {...getFilterBarTestId('collapsable')}
- className={cx({ open: !filtersOpen })}
- onClick={openFiltersBar}
- offset={offset}
- >
- <StyledCollapseIcon
- {...getFilterBarTestId('expand-button')}
- iconSize="l"
- />
- <StyledFilterIcon
- {...getFilterBarTestId('filter-icon')}
- iconSize="l"
- />
- </CollapsedBar>
- <Bar className={cx({ open: filtersOpen })} width={width}>
- <Header toggleFiltersBar={toggleFiltersBar} />
- {!isInitialized ? (
- <div css={{ height }}>
- <Loading />
- </div>
- ) : isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) ? (
- <StyledTabs
- centered
- onChange={setTab as HandlerFunction}
- defaultActiveKey={TabIds.AllFilters}
- activeKey={editFilterSetId ? TabIds.AllFilters : undefined}
- >
- <AntdTabs.TabPane
- tab={t('All filters (%(filterCount)d)', {
- filterCount: numberOfFilters,
- })}
- key={TabIds.AllFilters}
- css={tabPaneStyle}
- >
- {editFilterSetId && (
- <EditSection
- dataMaskSelected={dataMaskSelected}
- disabled={!isApplyDisabled}
- onCancel={() => setEditFilterSetId(null)}
- filterSetId={editFilterSetId}
- />
- )}
- {filterValues.length === 0 ? (
- <FilterBarEmptyStateContainer>
- <EmptyStateSmall
- title={t('No filters are currently added')}
- image="filter.svg"
- description={
- canEdit &&
- t(
- 'Click the button above to add a filter to the dashboard',
- )
- }
- />
- </FilterBarEmptyStateContainer>
- ) : (
- <FilterControls
- dataMaskSelected={dataMaskSelected}
- directPathToChild={directPathToChild}
- onFilterSelectionChange={handleFilterSelectionChange}
- />
- )}
- </AntdTabs.TabPane>
- <AntdTabs.TabPane
- disabled={!!editFilterSetId}
- tab={t('Filter sets (%(filterSetCount)d)', {
- filterSetCount: filterSetFilterValues.length,
- })}
- key={TabIds.FilterSets}
- css={tabPaneStyle}
- >
- <FilterSets
- onEditFilterSet={setEditFilterSetId}
- disabled={!isApplyDisabled}
- dataMaskSelected={dataMaskSelected}
- tab={tab}
- onFilterSelectionChange={handleFilterSelectionChange}
- />
- </AntdTabs.TabPane>
- </StyledTabs>
- ) : (
- <div css={tabPaneStyle} onScroll={onScroll}>
- {filterValues.length === 0 ? (
- <FilterBarEmptyStateContainer>
- <EmptyStateSmall
- title={t('No filters are currently added')}
- image="filter.svg"
- description={
- canEdit &&
- t(
- 'Click the button above to add a filter to the dashboard',
- )
- }
- />
- </FilterBarEmptyStateContainer>
- ) : (
- <FilterControls
- dataMaskSelected={dataMaskSelected}
- directPathToChild={directPathToChild}
- onFilterSelectionChange={handleFilterSelectionChange}
- />
- )}
- </div>
- )}
- <ActionButtons
- width={width}
- onApply={handleApply}
- onClearAll={handleClearAll}
- dataMaskSelected={dataMaskSelected}
- dataMaskApplied={dataMaskApplied}
- isApplyDisabled={isApplyDisabled}
- />
- </Bar>
- </BarWrapper>
- </FilterBarScrollContext.Provider>
+ const actions = useMemo(
+ () => (
+ <ActionButtons
+ orientation={orientation}
+ width={verticalConfig?.width}
+ onApply={handleApply}
+ onClearAll={handleClearAll}
+ dataMaskSelected={dataMaskSelected}
+ dataMaskApplied={dataMaskApplied}
+ isApplyDisabled={isApplyDisabled}
+ />
+ ),
+ [verticalConfig?.width, dataMaskSelected, dataMaskApplied, isApplyDisabled],
Review Comment:
We have many missing dependencies here. I suggest not using `useMemo` if we can't handle the dependencies because it will be using stale values.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -0,0 +1,298 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/* eslint-disable no-param-reassign */
+import throttle from 'lodash/throttle';
+import React, {
+ useEffect,
+ useState,
+ useCallback,
+ useMemo,
+ useRef,
+ createContext,
+} from 'react';
+import cx from 'classnames';
+import { HandlerFunction, styled, t, isNativeFilter } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import { AntdTabs } from 'src/components';
+import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
+import Loading from 'src/components/Loading';
+import { EmptyStateSmall } from 'src/components/EmptyState';
+import { TabIds, getFilterBarTestId, VerticalBarProps } from './utils';
+import FilterSets from './FilterSets';
+import { useFilterSets } from './state';
+import EditSection from './FilterSets/EditSection';
+import Header from './Header';
+import FilterControls from './FilterControls/FilterControls';
+
+const BarWrapper = styled.div<{ width: number }>`
+ width: ${({ theme }) => theme.gridUnit * 8}px;
+
+ & .ant-tabs-top > .ant-tabs-nav {
+ margin: 0;
+ }
+ &.open {
+ width: ${({ width }) => width}px; // arbitrary...
+ }
+`;
+
+const Bar = styled.div<{ width: number }>`
+ & .ant-typography-edit-content {
+ left: 0;
+ margin-top: 0;
+ width: 100%;
+ }
+ position: absolute;
+ top: 0;
+ left: 0;
+ flex-direction: column;
+ flex-grow: 1;
+ width: ${({ width }) => width}px;
+ background: ${({ theme }) => theme.colors.grayscale.light5};
+ border-right: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ min-height: 100%;
+ display: none;
+ &.open {
+ display: flex;
+ }
+`;
+
+const CollapsedBar = styled.div<{ offset: number }>`
+ position: absolute;
+ top: ${({ offset }) => offset}px;
+ left: 0;
+ height: 100%;
+ width: ${({ theme }) => theme.gridUnit * 8}px;
+ padding-top: ${({ theme }) => theme.gridUnit * 2}px;
+ display: none;
+ text-align: center;
+ &.open {
+ display: flex;
+ flex-direction: column;
+ align-items: center;
+ padding: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+ svg {
+ cursor: pointer;
+ }
+`;
+
+const StyledCollapseIcon = styled(Icons.Collapse)`
+ color: ${({ theme }) => theme.colors.primary.base};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 3}px;
+`;
+
+const StyledFilterIcon = styled(Icons.Filter)`
+ color: ${({ theme }) => theme.colors.grayscale.base};
+`;
+
+const StyledTabs = styled(AntdTabs)`
+ & .ant-tabs-nav-list {
+ width: 100%;
+ }
+ & .ant-tabs-tab {
+ display: flex;
+ justify-content: center;
+ margin: 0;
+ flex: 1;
+ }
+
+ & > .ant-tabs-nav .ant-tabs-nav-operations {
+ display: none;
+ }
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+ margin-top: ${({ theme }) => theme.gridUnit * 8}px;
+`;
+
+export const FilterBarScrollContext = createContext(false);
+const VerticalFilterBar: React.FC<VerticalBarProps> = ({
+ actions,
+ canEdit,
+ dataMaskSelected,
+ directPathToChild,
+ filtersOpen,
+ filterValues,
+ height,
+ isDisabled,
+ isInitialized,
+ offset,
+ onSelectionChange,
+ toggleFiltersBar,
+ width,
+}) => {
+ const [editFilterSetId, setEditFilterSetId] = useState<number | null>(null);
+ const filterSets = useFilterSets();
+ const filterSetFilterValues = Object.values(filterSets);
+ const [tab, setTab] = useState(TabIds.AllFilters);
+ const nativeFilterValues = filterValues.filter(isNativeFilter);
+ const [isScrolling, setIsScrolling] = useState(false);
+ const timeout = useRef<any>();
+
+ const openFiltersBar = useCallback(
+ () => toggleFiltersBar(true),
+ [toggleFiltersBar],
+ );
+
+ const onScroll = useCallback(
+ throttle(() => {
+ clearTimeout(timeout.current);
+ setIsScrolling(true);
+ timeout.current = setTimeout(() => {
+ setIsScrolling(false);
+ }, 300);
+ }, 200),
+ [],
+ );
Review Comment:
This is functionally equivalent without the warning.
```suggestion
const onScroll = useMemo(
() =>
throttle(() => {
clearTimeout(timeout.current);
setIsScrolling(true);
timeout.current = setTimeout(() => {
setIsScrolling(false);
}, 300);
}, 200),
[],
);
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts:
##########
@@ -18,7 +18,52 @@
*/
import { areObjectsEqual } from 'src/reduxUtils';
-import { DataMaskStateWithId, Filter, FilterState } from '@superset-ui/core';
+import {
+ DataMask,
+ DataMaskStateWithId,
+ Divider,
+ Filter,
+ FilterState,
+} from '@superset-ui/core';
+import { testWithId } from 'src/utils/testUtils';
+import { FilterBarLocation } from 'src/dashboard/types';
+
+interface CommonFiltersBarProps {
Review Comment:
Can we leave only utility functions in this file and move type definitions to a types file?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -0,0 +1,298 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/* eslint-disable no-param-reassign */
+import throttle from 'lodash/throttle';
+import React, {
+ useEffect,
+ useState,
+ useCallback,
+ useMemo,
+ useRef,
+ createContext,
+} from 'react';
+import cx from 'classnames';
+import { HandlerFunction, styled, t, isNativeFilter } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import { AntdTabs } from 'src/components';
+import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
+import Loading from 'src/components/Loading';
+import { EmptyStateSmall } from 'src/components/EmptyState';
+import { TabIds, getFilterBarTestId, VerticalBarProps } from './utils';
+import FilterSets from './FilterSets';
+import { useFilterSets } from './state';
+import EditSection from './FilterSets/EditSection';
+import Header from './Header';
+import FilterControls from './FilterControls/FilterControls';
+
+const BarWrapper = styled.div<{ width: number }>`
+ width: ${({ theme }) => theme.gridUnit * 8}px;
+
+ & .ant-tabs-top > .ant-tabs-nav {
+ margin: 0;
+ }
+ &.open {
+ width: ${({ width }) => width}px; // arbitrary...
+ }
+`;
+
+const Bar = styled.div<{ width: number }>`
+ & .ant-typography-edit-content {
+ left: 0;
+ margin-top: 0;
+ width: 100%;
+ }
+ position: absolute;
+ top: 0;
+ left: 0;
+ flex-direction: column;
+ flex-grow: 1;
+ width: ${({ width }) => width}px;
+ background: ${({ theme }) => theme.colors.grayscale.light5};
+ border-right: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ min-height: 100%;
+ display: none;
+ &.open {
+ display: flex;
+ }
+`;
+
+const CollapsedBar = styled.div<{ offset: number }>`
+ position: absolute;
+ top: ${({ offset }) => offset}px;
+ left: 0;
+ height: 100%;
+ width: ${({ theme }) => theme.gridUnit * 8}px;
+ padding-top: ${({ theme }) => theme.gridUnit * 2}px;
+ display: none;
+ text-align: center;
+ &.open {
+ display: flex;
+ flex-direction: column;
+ align-items: center;
+ padding: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+ svg {
+ cursor: pointer;
+ }
+`;
+
+const StyledCollapseIcon = styled(Icons.Collapse)`
+ color: ${({ theme }) => theme.colors.primary.base};
Review Comment:
Same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido merged pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
geido merged PR #22064:
URL: https://github.com/apache/superset/pull/22064
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #22064:
URL: https://github.com/apache/superset/pull/22064#issuecomment-1319405071
Regression test LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #22064:
URL: https://github.com/apache/superset/pull/22064#discussion_r1018207395
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -37,38 +38,15 @@ interface ActionButtonsProps {
dataMaskSelected: DataMaskState;
dataMaskApplied: DataMaskStateWithId;
isApplyDisabled: boolean;
+ orientation?: FilterBarLocation;
Review Comment:
Can we rename it to `FilterBarOrientation`?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -37,38 +38,15 @@ interface ActionButtonsProps {
dataMaskSelected: DataMaskState;
dataMaskApplied: DataMaskStateWithId;
isApplyDisabled: boolean;
+ orientation?: FilterBarLocation;
}
-const ActionButtonsContainer = styled.div<{ width: number }>`
- ${({ theme, width }) => css`
+const ActionButtonsContainer = styled.div<{
Review Comment:
Totally optional, but for these cases, I like to use [Emotion's Composition](https://emotion.sh/docs/composition).
You can write the individual styles:
```
const containerStyle = (theme: SupersetTheme) => css`
display: flex;
&& > .filter-clear-all-button {
color: ${theme.colors.grayscale.base};
margin-left: 0;
&:hover {
color: ${theme.colors.primary.dark1};
}
&[disabled],
&[disabled]:hover {
color: ${theme.colors.grayscale.light1};
}
}
`;
const verticalStyle = (theme: SupersetTheme, width: number) => css`
flex-direction: column;
align-items: center;
pointer-events: none;
position: fixed;
z-index: 100;
// filter bar width minus 1px for border
width: ${width - 1}px;
bottom: 0;
padding: ${theme.gridUnit * 4}px;
padding-top: ${theme.gridUnit * 6}px;
background: linear-gradient(
${rgba(theme.colors.grayscale.light5, 0)},
${theme.colors.grayscale.light5} ${theme.opacity.mediumLight}
);
& > button {
pointer-events: auto;
}
& > .filter-apply-button {
margin-bottom: ${theme.gridUnit * 3}px;
}
`;
const horizontalStyle = (theme: SupersetTheme) => css`
margin: 0 ${theme.gridUnit * 2}px;
&& > .filter-clear-all-button {
text-transform: capitalize;
font-weight: ${theme.typography.weights.normal};
}
& > .filter-apply-button {
&[disabled],
&[disabled]:hover {
color: ${theme.colors.grayscale.light1};
background: ${theme.colors.grayscale.light3};
}
}
`;
```
And apply them conditionally:
```
return (
<div
data-test="filterbar-action-buttons"
css={(theme: SupersetTheme) => [
containerStyle(theme),
isVertical ? verticalStyle(theme, width) : horizontalStyle(theme),
]}
>
...
</div>
);
};
```
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -354,6 +361,14 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
{!hideDashboardHeader && <DashboardHeader />}
+ {nativeFiltersEnabled &&
Review Comment:
Can you extract `nativeFiltersEnabled && !editMode` to a const called `showFilterBar` to be used in both places?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx:
##########
@@ -58,6 +58,10 @@ const HeaderButton = styled(Button)`
const Wrapper = styled.div`
padding: ${({ theme }) => theme.gridUnit}px
${({ theme }) => theme.gridUnit * 2}px;
+
+ .ant-dropdown-trigger span {
+ padding-right: ${({ theme }) => theme.gridUnit * 2}px;
+ }
`;
Review Comment:
```suggestion
const Wrapper = styled.div`
${({ theme }) => `
padding: ${theme.gridUnit}px ${theme.gridUnit * 2}px;
.ant-dropdown-trigger span {
padding-right: ${theme.gridUnit * 2}px;
}
`}
`;
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx:
##########
@@ -0,0 +1,107 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { NativeFilterType } from '@superset-ui/core';
+import React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import HorizontalBar from './Horizontal';
+
+describe('HorizontalFilterBar', () => {
Review Comment:
[Avoid nesting when you're testing](https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#avoid-nesting-when-youre-testing) 😉
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+ background: ${theme.colors.grayscale.light5};
+ box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+ ${({ theme }) => `
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ justify-content: flex-start;
+ padding: 0 ${theme.gridUnit * 2}px;
+
+ .loading {
+ margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+ padding: 0;
+ }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+ ${({ theme }) => `
+ margin: 0 ${theme.gridUnit * 2}px;
+ font-weight: ${theme.typography.weights.bold};
+`}
+`;
+
+const FiltersLinkContainer = styled.div<{ hasFilters: boolean }>`
+ ${({ theme, hasFilters }) => `
+ padding: 0 ${theme.gridUnit * 2}px;
+ border-right: ${
+ hasFilters ? `1px solid ${theme.colors.grayscale.light2}` : 0
+ };
+
+ button {
+ display: flex;
+ align-items: center;
+ text-transform: capitalize;
+ font-weight: ${theme.typography.weights.normal};
+ color: ${theme.colors.primary.base};
+ > .anticon + span, > .anticon {
+ margin-right: 0;
+ margin-left: 0;
+ }
+ }
+`}
+`;
Review Comment:
```suggestion
const HorizontalBar = styled.div`
${({ theme }) => `
padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
background: ${theme.colors.grayscale.light5};
box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
`}
`;
const HorizontalBarContent = styled.div`
${({ theme }) => `
display: flex;
flex-direction: row;
flex-wrap: nowrap;
align-items: center;
justify-content: flex-start;
padding: 0 ${theme.gridUnit * 2}px;
.loading {
margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
padding: 0;
}
`}
`;
const FilterBarEmptyStateContainer = styled.div`
${({ theme }) => `
margin: 0 ${theme.gridUnit * 2}px;
font-weight: ${theme.typography.weights.bold};
`}
`;
const FiltersLinkContainer = styled.div<{ hasFilters: boolean }>`
${({ theme, hasFilters }) => `
padding: 0 ${theme.gridUnit * 2}px;
border-right: ${
hasFilters ? `1px solid ${theme.colors.grayscale.light2}` : 0
};
button {
display: flex;
align-items: center;
text-transform: capitalize;
font-weight: ${theme.typography.weights.normal};
color: ${theme.colors.primary.base};
> .anticon + span, > .anticon {
margin-right: 0;
margin-left: 0;
}
}
`}
`;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #22064: feat: Horizontal filter bar states
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #22064:
URL: https://github.com/apache/superset/pull/22064#discussion_r1018328967
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarLocationSelect/index.tsx:
##########
@@ -62,7 +62,7 @@ export const FilterBarLocationSelect = () => {
return (
<DropdownSelectableIcon
Review Comment:
It looks like this component gives itself a line height that stops it from looking quite vertically aligned in the flex container – would it be possible to override that in horizontal mode?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+ background: ${theme.colors.grayscale.light5};
+ box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+ ${({ theme }) => `
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ justify-content: flex-start;
+ padding: 0 ${theme.gridUnit * 2}px;
+
+ .loading {
+ margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+ padding: 0;
+ }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+ ${({ theme }) => `
+ margin: 0 ${theme.gridUnit * 2}px;
Review Comment:
Two design nits:
- I think according to the designs, distance between the gear and this message should be 18px.
- It looks like it should be size 12px (`s`) and color #666666 (`greyscale.base`)
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+ background: ${theme.colors.grayscale.light5};
+ box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+ ${({ theme }) => `
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ justify-content: flex-start;
+ padding: 0 ${theme.gridUnit * 2}px;
+
+ .loading {
+ margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+ padding: 0;
+ }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+ ${({ theme }) => `
+ margin: 0 ${theme.gridUnit * 2}px;
+ font-weight: ${theme.typography.weights.bold};
+`}
+`;
+
+const FiltersLinkContainer = styled.div<{ hasFilters: boolean }>`
+ ${({ theme, hasFilters }) => `
+ padding: 0 ${theme.gridUnit * 2}px;
+ border-right: ${
+ hasFilters ? `1px solid ${theme.colors.grayscale.light2}` : 0
+ };
+
+ button {
+ display: flex;
+ align-items: center;
+ text-transform: capitalize;
+ font-weight: ${theme.typography.weights.normal};
+ color: ${theme.colors.primary.base};
+ > .anticon + span, > .anticon {
+ margin-right: 0;
+ margin-left: 0;
+ }
+ }
+`}
+`;
+
+const HorizontalFilterBar: React.FC<HorizontalBarProps> = ({
+ actions,
+ canEdit,
+ dashboardId,
+ dataMaskSelected,
+ filterValues,
+ isInitialized,
+ directPathToChild,
+ onSelectionChange,
+}) => {
+ const hasFilters = filterValues.length > 0;
+
+ return (
+ <HorizontalBar {...getFilterBarTestId()}>
+ <HorizontalBarContent>
+ {!isInitialized ? (
+ <Loading position="inline-centered" />
+ ) : (
+ <>
+ {canEdit && <FilterBarLocationSelect />}
+ {!hasFilters && (
+ <FilterBarEmptyStateContainer>
+ {t('No filters are currently added to this dashboard.')}
+ </FilterBarEmptyStateContainer>
+ )}
+ {canEdit && (
+ <FiltersLinkContainer hasFilters={hasFilters}>
+ <FilterConfigurationLink
+ dashboardId={dashboardId}
+ createNewOnOpen={filterValues.length === 0}
+ >
+ <Icons.PlusSmall /> {t('Add/Edit Filters')}
Review Comment:
Another design nit: I think the plus icon has some padding or something stopping it from being vertically-centered, and it looks maybe a little closer to the Add/Edit Filters text than it should (~12px).
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarLocationSelect/index.tsx:
##########
@@ -62,7 +62,7 @@ export const FilterBarLocationSelect = () => {
return (
<DropdownSelectableIcon
Review Comment:
As-is:
<img width="608" alt="Screen Shot 2022-11-09 at 12 22 16 PM" src="https://user-images.githubusercontent.com/13007381/200922119-5f30afce-14e4-437a-83e3-d971ae957a05.png">
With `line-height: 0;`:
<img width="671" alt="Screen Shot 2022-11-09 at 12 23 00 PM" src="https://user-images.githubusercontent.com/13007381/200922209-b22a1532-1d7f-43ca-88c0-420c00eb6659.png">
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+ background: ${theme.colors.grayscale.light5};
+ box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+ ${({ theme }) => `
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ justify-content: flex-start;
+ padding: 0 ${theme.gridUnit * 2}px;
+
+ .loading {
+ margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+ padding: 0;
+ }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+ ${({ theme }) => `
+ margin: 0 ${theme.gridUnit * 2}px;
Review Comment:
Design:
<img width="358" alt="Screen Shot 2022-11-09 at 12 31 13 PM" src="https://user-images.githubusercontent.com/13007381/200923724-50ee233d-dfc1-4c79-a845-9bd014240176.png">
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+ background: ${theme.colors.grayscale.light5};
+ box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+ ${({ theme }) => `
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ justify-content: flex-start;
+ padding: 0 ${theme.gridUnit * 2}px;
+
+ .loading {
+ margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+ padding: 0;
+ }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+ ${({ theme }) => `
+ margin: 0 ${theme.gridUnit * 2}px;
+ font-weight: ${theme.typography.weights.bold};
+`}
+`;
+
+const FiltersLinkContainer = styled.div<{ hasFilters: boolean }>`
+ ${({ theme, hasFilters }) => `
+ padding: 0 ${theme.gridUnit * 2}px;
+ border-right: ${
+ hasFilters ? `1px solid ${theme.colors.grayscale.light2}` : 0
+ };
+
+ button {
+ display: flex;
+ align-items: center;
+ text-transform: capitalize;
+ font-weight: ${theme.typography.weights.normal};
+ color: ${theme.colors.primary.base};
+ > .anticon + span, > .anticon {
+ margin-right: 0;
+ margin-left: 0;
+ }
+ }
+`}
+`;
+
+const HorizontalFilterBar: React.FC<HorizontalBarProps> = ({
+ actions,
+ canEdit,
+ dashboardId,
+ dataMaskSelected,
+ filterValues,
+ isInitialized,
+ directPathToChild,
+ onSelectionChange,
+}) => {
+ const hasFilters = filterValues.length > 0;
+
+ return (
+ <HorizontalBar {...getFilterBarTestId()}>
+ <HorizontalBarContent>
+ {!isInitialized ? (
+ <Loading position="inline-centered" />
+ ) : (
+ <>
+ {canEdit && <FilterBarLocationSelect />}
+ {!hasFilters && (
+ <FilterBarEmptyStateContainer>
+ {t('No filters are currently added to this dashboard.')}
+ </FilterBarEmptyStateContainer>
+ )}
+ {canEdit && (
+ <FiltersLinkContainer hasFilters={hasFilters}>
+ <FilterConfigurationLink
+ dashboardId={dashboardId}
+ createNewOnOpen={filterValues.length === 0}
+ >
+ <Icons.PlusSmall /> {t('Add/Edit Filters')}
Review Comment:
Design:
<img width="133" alt="Screen Shot 2022-11-09 at 12 33 37 PM" src="https://user-images.githubusercontent.com/13007381/200924236-18b1b70c-9feb-48ef-a3cd-71806d50a683.png">
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -37,38 +38,15 @@ interface ActionButtonsProps {
dataMaskSelected: DataMaskState;
dataMaskApplied: DataMaskStateWithId;
isApplyDisabled: boolean;
+ orientation?: FilterBarLocation;
Review Comment:
Can we choose one of position vs. placement vs. orientation vs. location and use that everywhere (Redux, props, filenames, etc)? I think if we go with orientation, vertical and horizontal make sense as the options, but if we go with position/placement/location I think top and left make more sense as the options.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -354,6 +361,14 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
{!hideDashboardHeader && <DashboardHeader />}
+ {nativeFiltersEnabled &&
+ !editMode &&
+ filterBarLocation === FilterBarLocation.HORIZONTAL && (
+ <FilterBar
+ orientation={FilterBarLocation.HORIZONTAL}
+ directPathToChild={directPathToChild}
Review Comment:
Getting a missing dependency warning about `directPathToChild`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org