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/02/18 20:25:05 UTC
[GitHub] [superset] yardz opened a new pull request #13225: fix: missing key on welcome page
yardz opened a new pull request #13225:
URL: https://github.com/apache/superset/pull/13225
### SUMMARY
There is an error on ActivityTable.tsx
<img width="1680" alt="Captura de Tela 2021-02-18 às 5 12 54 PM" src="https://user-images.githubusercontent.com/1014611/108416513-91ba9e80-720d-11eb-9400-5946f527222c.png">
I refactored the component because it was too big and it was difficult to debug (and to make it easier to create tests in the future)
I divided it into a second component, and fixed the key problem.
<img width="1678" alt="Captura de Tela 2021-02-18 às 5 11 35 PM" src="https://user-images.githubusercontent.com/1014611/108416752-e8c07380-720d-11eb-849b-d5ac72377995.png">
### TEST PLAN
Tests should pass normally. No application behavior has been changed.
The ActivityTable.tsx interface has been maintained, and the file has not been moved.
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580192926
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
Review comment:
This solved the problem of "not unique" keys.
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580717304
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
If it does not impact the users, then there is no urgency to merge your fix. I personally feel typing is a bigger finish to catch than solving a developer warning that is not visible to users. You may disagree but there is no need to be sarcastic.
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580707080
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
+ key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}
Review comment:
That's unfortunate and probably another reason why we want to do the typing refactoring first. It really isn't that hard to do because most of the types already exist somewhere else. See: https://github.com/apache/superset/pull/13291
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580545531
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
+ key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}
Review comment:
If `id` is always present, then `dashboard_title` is probably not needed. Feel free to update the typing based on API responses.
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580554310
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
+ key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}
Review comment:
@ktmud That's the problem, the `id` is **not always present**. On the main tab page it does not exist. In the other tabs it exists (with the test data). This typing is not reliable.
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580063835
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
`TableRow` sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other `XxxTable` to simply `Recents`, `Dashboards`, `Charts` and `SavedQueries` to be consistent with section titles, then the single tab content can be named as `RecentsList`, `DashboardList`, etc.
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580181506
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
> `TableRow` sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other `XxxTable` to simply `Recents`, `Dashboards`, `Charts` and `SavedQueries` to be consistent with section titles, then the single table content can be named as `RecentsList`, `DashboardList`, etc.
I agree that the names are not good. But I think this is a task for another PR, the purpose of this was just to solve the problem of the keys and not to solve "all problems" related to these files.
Example: These files also need test files, the types need to be better defined, etc.
None of this is the scope of this PR, just as renaming the "ActivityTable" does not seem to me to be scope for this PR.
@ktmud
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580197136
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
+ key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}
Review comment:
These types need a better definition. Several "mandatory" fields are present only sometimes (example, id) ...
This definition is not at all reliable...
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580603980
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
@ktmud Do you prefer a buggy code and a more complex code than a simpler and no bug code, because the type is not perfect?
Sorry it doesn't make any sense to me.
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580720814
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
> 1)we are not fixing UI bug, meaning I wouldn't notice it as an end user.
This is not necessarily true. Just because our test data set is not generating a noticeable error does not mean that with a different data set this error is imperceptible. This key problem generates a lot of errors when a component is rerendered.
> 2)there might be a more comprehensive solution to solve this issue.
From what I saw, there are some requests that are not correctly typed (and are in js). A definitive and responsible solution would involve much more work and several PRs. I wouldn't risk putting a "definitive" type without having done this work before it...
I believe that this component needs some refinement "cycles" to achieve the "definitive" result
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580547730
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
We should probably have separate type for each entity, then use union type for "activity object".
```
type ActivityObject = RecentSlice | RecentDashboard
```
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580556124
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
I agree, but again, this PR is not to improve the existing typing but to solve a bug. Refactoring and any other type of improvement is not within the scope of this PR.
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580181506
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
> `TableRow` sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other `XxxTable` to simply `Recents`, `Dashboards`, `Charts` and `SavedQueries` to be consistent with section titles, then the single table content can be named as `RecentsList`, `DashboardList`, etc.
I agree that the names are not good. But I think this is a task for another PR, the purpose of this was just to solve the problem of the keys and not to solve "all problems" related to these files.
Example: These files also need test files, the types need to be better defined, etc.
None of this is the scope of this PR, just as renaming the "ActivityTable" does not seem to me to be scope for this PR.
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580193840
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
+ key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}
Review comment:
This solved the problem of repeated keys.
----------------------------------------------------------------
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
[GitHub] [superset] junlincc commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
junlincc commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580716381
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
Hey guys sorry i haven't been following the threads. based on the conversation above, my understanding is that
1)we are not fixing UI bug, meaning I wouldn't notice it as an end user. 2)there might be a more comprehensive solution to solve this issue.
@yardz I prioritize code quality + cleaning up technical debt in Q1-Q2, more than even fixing an actual UI bug. Let's make sure if refractor is necessary, we are not simply putting a bandaid on the code, but solve it from the root, also not get distracted by not prioritized tasks.
would you mind addressing @ktmud comment if that's not too much of work, or we can park this PR and move on.
cc @rusackas
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580505111
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
@ktmud
----------------------------------------------------------------
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
[GitHub] [superset] junlincc commented on pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13225:
URL: https://github.com/apache/superset/pull/13225#issuecomment-781627357
Thanks for being proactive and cleaning up the code as you move, Bruno! ♥️ @yardz
@pkdotson can you review this PR since you are the owner? thanks!
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580717304
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
If it does not impact the users, then there is no urgency to merge your fix. I personally feel typing is a bigger fish to catch than solving a developer warning that is not visible to users. You may disagree but there is no need to be sarcastic.
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580699253
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
Please take a look at https://github.com/apache/superset/pull/13291
1. Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).
2. Current code wasn't buggy to the end users ((for the key problem at least).
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
Please take a look at https://github.com/apache/superset/pull/13291
1. Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).
2. Current code wasn't buggy to the end users (on the key problem at least).
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580707758
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
> 1. Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).
I didn't talk about the type but about the original component, which had 1 more component inside it. (2 components in 1).
This is a more complex component and much more difficult to test.
> 2. Current code (for React key at least) wasn't buggy to the end users.
If the user didn't see it then the bug doesn't exist?
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r578879975
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
I gave this name following the logic "table->row". If you look at ActivityTableRow it receives an array and returns several "cards" inside a Fragment.
I had thought of "ActivityList" (list of cards right?), but I chose to keep it as "ActivityTableRow" as there are some improvements to be made in the props and these components are very related...
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13225:
URL: https://github.com/apache/superset/pull/13225#issuecomment-784016328
Supersedes by #13291
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580588319
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
This one could be related since accurate typing may have an impact on how unique keys are chosen. Since this is not a user-facing bug, it might also make sense to do the type refactoring first (either in this or another PR) before merging this fix.
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580195020
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
+ key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}
Review comment:
@ktmud The "infinite cards bug" is a problem with keys (or lack thereof).
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580193840
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
<>
{activityList.map(activity => (
<CardStyles
- key={`${activity.item_title}__${activity.time}`}
+ key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}
Review comment:
This solved the problem of "not unique" keys.
----------------------------------------------------------------
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
[GitHub] [superset] ktmud closed pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud closed pull request #13225:
URL: https://github.com/apache/superset/pull/13225
----------------------------------------------------------------
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
[GitHub] [superset] yardz commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580556124
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
I agree, but again, this PR is not to improve the existing types but to solve a bug. Refactoring and any other type of improvement is not within the scope of this PR.
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580546142
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
Agreed.
----------------------------------------------------------------
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
[GitHub] [superset] pkdotson commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
pkdotson commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r578764406
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
nit: Does it makes sense to call this ActivityCard ?
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r579774509
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -17,31 +17,16 @@
* under the License.
*/
import React from 'react';
-import moment from 'moment';
+
import { styled, t } from '@superset-ui/core';
import Loading from 'src/components/Loading';
-import ListViewCard from 'src/components/ListViewCard';
import SubMenu from 'src/components/Menu/SubMenu';
+import ActivityTableRow from './ActivityTableRow';
import { ActivityData } from './Welcome';
-import { mq, CardStyles } from '../utils';
+import { mq } from '../utils';
Review comment:
We prefer absolute import path.
```suggestion
import { mq } from 'src/views/CURD/utils';
```
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
- <ActivityContainer>{renderActivity()}</ActivityContainer>
+ <ActivityContainer>
+ <ActivityTableRow
Review comment:
`TableRow` sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other `XxxTable` to simply `Recents`, `Dashboards`, `Charts` and `SavedQueries` to be consistent with section titles, then the single table content can be named as `RecentsList`, `DashboardList`, etc.
----------------------------------------------------------------
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
[GitHub] [superset] ktmud commented on a change in pull request #13225: fix: missing key on welcome page
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13225:
URL: https://github.com/apache/superset/pull/13225#discussion_r580699253
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 moment from 'moment';
+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {
Review comment:
Please take a look at https://github.com/apache/superset/pull/13291
1. Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).
2. Current code (for React key at least) wasn't buggy to the end users.
----------------------------------------------------------------
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