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 2021/10/27 17:50:48 UTC
[GitHub] [superset] suddjian commented on a change in pull request #17175: feat [WIP]: embedded dashboard
suddjian commented on a change in pull request #17175:
URL: https://github.com/apache/superset/pull/17175#discussion_r737692587
##########
File path: superset-frontend/src/constants.ts
##########
@@ -27,6 +27,10 @@ export const URL_PARAMS = {
name: 'standalone',
type: 'number',
},
+ embedded: {
Review comment:
Let's add a comment here about how this works
##########
File path: superset-frontend/src/components/Emmbedded/index.tsx
##########
@@ -0,0 +1,57 @@
+/**
+ * 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, { createContext, useContext, useState } from 'react';
+
+interface EmbeddedConfigType {
+ hideTitle: boolean;
+ hideTab: boolean;
+ hideNav: boolean;
+ hideChartFilter: boolean;
+}
+interface EmbeddedProviderProps {
+ children: JSX.Element;
+ config: number;
+}
+
+export const EmbeddedContext = createContext<EmbeddedConfigType>({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartFilter: false,
+});
+
+export const useEmbedded = () => useContext(EmbeddedContext);
Review comment:
and let's call this `useUiConfig`
##########
File path: superset-frontend/src/components/Emmbedded/index.tsx
##########
@@ -0,0 +1,57 @@
+/**
+ * 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, { createContext, useContext, useState } from 'react';
+
+interface EmbeddedConfigType {
+ hideTitle: boolean;
+ hideTab: boolean;
+ hideNav: boolean;
+ hideChartFilter: boolean;
+}
+interface EmbeddedProviderProps {
+ children: JSX.Element;
+ config: number;
+}
+
+export const EmbeddedContext = createContext<EmbeddedConfigType>({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartFilter: false,
+});
+
+export const useEmbedded = () => useContext(EmbeddedContext);
+
+export const EmbeddedProvider: React.FC<EmbeddedProviderProps> = ({
Review comment:
We can call this `EmbeddedUiConfigProvider`. In the future we could have `FullUiConfigProvider`, and potentially others that provide the `UiConfigContext` in different ways.
##########
File path: superset-frontend/src/components/Emmbedded/index.tsx
##########
@@ -0,0 +1,57 @@
+/**
+ * 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, { createContext, useContext, useState } from 'react';
+
+interface EmbeddedConfigType {
+ hideTitle: boolean;
+ hideTab: boolean;
+ hideNav: boolean;
+ hideChartFilter: boolean;
+}
+interface EmbeddedProviderProps {
+ children: JSX.Element;
+ config: number;
+}
+
+export const EmbeddedContext = createContext<EmbeddedConfigType>({
Review comment:
I think we should call this `UiConfigContext`, so that it can be used for things other than embedded dashboards. I think that better describes what this context is to be used for.
##########
File path: superset-frontend/src/components/Emmbedded/index.tsx
##########
@@ -0,0 +1,57 @@
+/**
+ * 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, { createContext, useContext, useState } from 'react';
+
+interface EmbeddedConfigType {
+ hideTitle: boolean;
+ hideTab: boolean;
+ hideNav: boolean;
+ hideChartFilter: boolean;
+}
+interface EmbeddedProviderProps {
+ children: JSX.Element;
+ config: number;
+}
+
+export const EmbeddedContext = createContext<EmbeddedConfigType>({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartFilter: false,
+});
+
+export const useEmbedded = () => useContext(EmbeddedContext);
+
+export const EmbeddedProvider: React.FC<EmbeddedProviderProps> = ({
+ children,
+ config,
+}) => {
+ const formattedConfig = (config >>> 0).toString(2);
+ const [embeddedConfig] = useState({
+ hideTitle: formattedConfig[0] === '1',
+ hideTab: formattedConfig[1] === '1',
+ hideNav: formattedConfig[2] === '1',
+ hideChartFilter: formattedConfig[3] === '1',
Review comment:
Also can we call the last one `hideChartControls` since it covers both the kebab menu and the filter?
##########
File path: superset-frontend/src/components/Emmbedded/index.tsx
##########
@@ -0,0 +1,57 @@
+/**
Review comment:
typo in the directory
Also, given the other suggested naming changes below, we should probably name the directory `UiConfigContext`.
##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -63,19 +66,23 @@ const RootContextProviders: React.FC = ({ children }) => {
lastLocationPathname = location.pathname;
}, [location.pathname]);
+ const config = getUrlParam(URL_PARAMS.embedded);
Review comment:
Suggestion: I would move this line into the `EmbeddedProvider` for better encapsulation. And/or make the prop optional and use `getUrlParam` by default if no config is provided.
##########
File path: superset-frontend/src/components/Emmbedded/index.tsx
##########
@@ -0,0 +1,57 @@
+/**
+ * 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, { createContext, useContext, useState } from 'react';
+
+interface EmbeddedConfigType {
+ hideTitle: boolean;
+ hideTab: boolean;
+ hideNav: boolean;
+ hideChartFilter: boolean;
+}
+interface EmbeddedProviderProps {
+ children: JSX.Element;
+ config: number;
+}
+
+export const EmbeddedContext = createContext<EmbeddedConfigType>({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartFilter: false,
+});
+
+export const useEmbedded = () => useContext(EmbeddedContext);
+
+export const EmbeddedProvider: React.FC<EmbeddedProviderProps> = ({
+ children,
+ config,
+}) => {
+ const formattedConfig = (config >>> 0).toString(2);
+ const [embeddedConfig] = useState({
+ hideTitle: formattedConfig[0] === '1',
+ hideTab: formattedConfig[1] === '1',
+ hideNav: formattedConfig[2] === '1',
+ hideChartFilter: formattedConfig[3] === '1',
Review comment:
This is a more standard way to do bit flags:
```suggestion
hideTitle: config & 1 !== 0,
hideTab: config & 2 !== 0,
hideNav: config & 4 !== 0,
hideChartFilter: config & 8 !== 0,
```
Also, those numbers could be moved out into named constants to increase clarity.
That said, depending on the embedded dashboard implementation we might want to switch to named flags instead of bit flags.
--
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