[GitHub] [superset] geido commented on a change in pull request #14174: chore: Rewrites dashboard IconButton component

[GitBox <gi...@apache.org>]

geido commented on a change in pull request #14174:
URL: https://github.com/apache/superset/pull/14174#discussion_r627479135



##########
File path: superset-frontend/src/dashboard/components/IconButton.tsx
##########
@@ -0,0 +1,59 @@
+/**
+ * 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, { MouseEventHandler } from 'react';
+import Icons from 'src/components/Icons';
+import { SupersetTheme } from '@superset-ui/core';
+import * as AntdIcons from '@ant-design/icons/lib/icons';
+
+interface IconButtonProps {
+  icon: keyof typeof Icons | keyof typeof AntdIcons;

Review comment:
       I think we can declare this somewhere in the Icons directory so that it can be re-used without having to redefine it all the times

##########
File path: superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
##########
@@ -194,7 +201,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
                     shouldFocus={shouldFocusTabs}
                     menuItems={[
                       <IconButton
-                        className="fa fa-level-down"
+                        icon="FallOutlined"

Review comment:
       Just an idea. Wouldn't be better to pass the icon down directly instead of the name reference? In that way, if I want a button to have an icon of a smaller size or a different color, I can do it straight.

##########
File path: superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
##########
@@ -61,12 +61,19 @@ const HEADER_HEIGHT = 67;
 
 type DashboardBuilderProps = {};
 
-const StyledDashboardContent = styled.div<{ dashboardFiltersOpen: boolean }>`
+const StyledDashboardContent = styled.div<{
+  dashboardFiltersOpen: boolean;
+  editMode: boolean;
+}>`
   display: flex;
   flex-direction: row;
   flex-wrap: nowrap;
   height: auto;
   flex-grow: 1;
+  padding-left: ${({ theme, editMode }) => {

Review comment:
       Just a nit. Maybe just go straight to return for the sake of brevity

##########
File path: superset-frontend/src/dashboard/components/IconButton.tsx
##########
@@ -0,0 +1,59 @@
+/**
+ * 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, { MouseEventHandler } from 'react';
+import Icons from 'src/components/Icons';
+import { SupersetTheme } from '@superset-ui/core';
+import * as AntdIcons from '@ant-design/icons/lib/icons';
+
+interface IconButtonProps {
+  icon: keyof typeof Icons | keyof typeof AntdIcons;
+  label?: string;
+  onClick: MouseEventHandler<HTMLDivElement>;
+}
+
+const IconButton = ({ icon, label, onClick }: IconButtonProps) => {
+  const Icon = Icons[icon];
+  return (
+    <div
+      tabIndex={0}
+      role="button"
+      css={(theme: SupersetTheme) => ({

Review comment:
       We still have that discussion to do about using the css approach. Would you like to organize that conversation? I am curious to here your opinions. Maybe we can open a thread with a wider audience in the chat




-- 
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.

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