geido commented on code in PR #33517: URL: https://github.com/apache/superset/pull/33517#discussion_r2166569285
########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomPopover.tsx: ########## @@ -0,0 +1,113 @@ +/** + * 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 { useEffect, useRef, useState, cloneElement } from 'react'; +import { styled } from '@superset-ui/core'; + +const PopoverWrapper = styled.div` + position: relative; + display: inline-block; +`; + +const PopoverContainer = styled.div` + ${({ theme }) => ` + position: fixed; + background: ${theme.colors.grayscale.light4}; + border: 1px solid ${theme.colors.grayscale.light2}; + border-radius: ${theme.borderRadius}px; + box-shadow: 0 ${theme.sizeUnit / 2}px ${theme.sizeUnit * 2}px ${theme.colors.grayscale.light1}40; Review Comment: What does this component do that requires a custom implementation? Can we use an Ant Design vanilla component for this so that we can avoid style customizations as well? ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/SearchSelectDropdown.tsx: ########## @@ -0,0 +1,55 @@ +/** + * 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 import/no-extraneous-dependencies */ +import { styled } from '@superset-ui/core'; +import { Select } from 'antd'; Review Comment: As we moved all our components to superset ui core, should we bring in the official select component instead of importing directly from antd? ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/styles/ag-grid.css: ########## @@ -0,0 +1,6924 @@ +/* eslint-disable camelcase */ +/** + * 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. + */ +.ag-measurement-container { Review Comment: I haven't looked into this css very deeply but it makes me wonder whether themability works with this now that the theming work is part of master ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/Pagination.tsx: ########## @@ -0,0 +1,216 @@ +/** + * 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 theme-colors/no-literal-colors */ +import { styled, t } from '@superset-ui/core'; +import { + VerticalLeftOutlined, + VerticalRightOutlined, + LeftOutlined, + RightOutlined, +} from '@ant-design/icons'; + +const PaginationContainer = styled.div` Review Comment: I am surprised that we need to build a custom pagination component. What is the driving decision behind this? If we really need to, should we use a standard Ant Design pagination component? https://ant.design/components/pagination ########## superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts: ########## @@ -60,6 +60,7 @@ export enum FeatureFlag { SlackEnableAvatars = 'SLACK_ENABLE_AVATARS', EnableDashboardScreenshotEndpoints = 'ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS', EnableDashboardDownloadWebDriverScreenshot = 'ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT', + TableV2TimeComparisonEnabled = 'TABLE_V2_TIME_COMPARISON_ENABLED', Review Comment: @amaannawab923 should feature flags also now be the same across Superset / Preset? ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomHeader.tsx: ########## @@ -0,0 +1,262 @@ +/* eslint-disable jsx-a11y/no-static-element-interactions */ +/* eslint-disable theme-colors/no-literal-colors */ +/* + * 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 { useRef, useState } from 'react'; +import { styled, t } from '@superset-ui/core'; +import { ArrowDownOutlined, ArrowUpOutlined } from '@ant-design/icons'; +import CustomPopover from './CustomPopover'; +import FilterIcon from './Filter'; +import KebabMenu from './KebabMenu'; +import { + CustomColDef, + CustomHeaderParams, + SortState, + UserProvidedColDef, +} from '../../types'; + +// Styled Components +const Container = styled.div` + ${({ theme }) => ` + display: flex; + width: 100%; + + .three-dots-menu { + align-self: center; + margin-left: ${theme.sizeUnit}px; + cursor: pointer; + padding: ${theme.sizeUnit / 2}px; + border-radius: ${theme.borderRadius}px; + } + `} +`; + +const HeaderContainer = styled.div` + ${({ theme }) => ` + width: 100%; + display: flex; + align-items: center; + cursor: pointer; + padding: 0 ${theme.sizeUnit * 2}px; + overflow: hidden; + `} +`; + +const HeaderLabel = styled.span` + ${({ theme }) => ` + font-weight: ${theme.fontWeightStrong}; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + display: block; + max-width: 100%; + `} +`; + +const SortIconWrapper = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + margin-left: ${theme.sizeUnit * 2}px; + `} +`; + +const FilterIconWrapper = styled.div` + align-self: flex-end; + margin-left: auto; + cursor: pointer; +`; + +const MenuContainer = styled.div` + ${({ theme }) => ` + min-width: ${theme.sizeUnit * 45}px; + padding: ${theme.sizeUnit}px 0; + + .menu-item { + padding: ${theme.sizeUnit * 2}px ${theme.sizeUnit * 4}px; + cursor: pointer; + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + + &:hover { + background-color: ${theme.colors.primary.light4}; + } + } + + .menu-divider { + height: 1px; + background-color: ${theme.colors.grayscale.light2}; + margin: ${theme.sizeUnit}px 0; + } + `} +`; + +const getSortIcon = ( + sortState: SortState[], + colId: string | null, +): React.ReactNode => { + if (!sortState?.length || !colId) return null; + const currentSort = sortState[0]; + if (currentSort.colId === colId) { + if (currentSort.sort === 'asc') return <ArrowUpOutlined />; + if (currentSort.sort === 'desc') return <ArrowDownOutlined />; + } + return null; +}; + +const CustomHeader: React.FC<CustomHeaderParams> = ({ + displayName, + enableSorting, + setSort, + context, + column, + api, +}) => { + const { initialSortState, onColumnHeaderClicked } = context; + const colId = column?.getColId(); + const isPercentMetric = (column?.getColDef() as CustomColDef)?.context + ?.isPercentMetric; + const [isPopoverVisible, setPopoverVisible] = useState(false); + const filterContainerRef = useRef<HTMLDivElement>(null); + const [isMenuVisible, setIsMenuVisible] = useState(false); + const currentSort = initialSortState?.[0]; + + const userColumn = column.getUserProvidedColDef() as UserProvidedColDef; + const isMain = userColumn?.isMain; + const timeComparisonKey = userColumn?.timeComparisonKey || ''; + const sortKey = isMain ? colId.replace('Main', '').trim() : colId; + const isTimeComparison = !isMain && timeComparisonKey; + + const clearSort = () => { + onColumnHeaderClicked({ column: { colId: sortKey, sort: null } }); + setSort(null, false); + }; + + const handleSortAsc = () => { + onColumnHeaderClicked({ column: { colId: sortKey, sort: 'asc' } }); + setSort('asc', false); + }; + + const handleSortDesc = () => { + onColumnHeaderClicked({ column: { colId: sortKey, sort: 'desc' } }); + setSort('desc', false); + }; Review Comment: Can we dry this up a bit? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
