-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: missing key on welcome page #13225
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably have separate type for each entity, then use union type for "activity object".
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please take a look at #13291
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I didn't talk about the type but about the original component, which had 1 more component inside it. (2 components in 1).
If the user didn't see it then the bug doesn't exist? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey guys sorry i haven't been following the threads. based on the conversation above, my understanding is that cc @rusackas There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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... |
||
action?: string; | ||
item_title?: string; | ||
slice_name: string; | ||
time: string; | ||
changed_on_utc: string; | ||
url: string; | ||
sql: string; | ||
dashboard_title: string; | ||
label: string; | ||
id: string; | ||
table: object; | ||
item_url: string; | ||
} | ||
|
||
interface ActivityProps { | ||
activeChild: string; | ||
loading: boolean; | ||
activityList: ActivityObjects[]; | ||
} | ||
|
||
const getRecentRef = (activeChild: string, activity: ActivityObjects) => { | ||
if (activeChild === 'Viewed') { | ||
return activity.item_url; | ||
} | ||
return activity.sql | ||
? `/superset/sqllab?savedQueryId=${activity.id}` | ||
: activity.url; | ||
}; | ||
const getFilterTitle = (activity: ActivityObjects) => { | ||
if (activity.dashboard_title) return activity.dashboard_title; | ||
if (activity.label) return activity.label; | ||
if (activity.url && !activity.table) return activity.item_title; | ||
if (activity.item_title) return activity.item_title; | ||
return activity.slice_name; | ||
}; | ||
|
||
const getIconName = (activity: ActivityObjects) => { | ||
if (activity.sql) return 'sql'; | ||
if ( | ||
activity.url?.includes('dashboard') || | ||
activity.item_url?.includes('dashboard') | ||
) { | ||
return 'nav-dashboard'; | ||
} | ||
if ( | ||
activity.url?.includes('explore') || | ||
activity.item_url?.includes('explore') | ||
) { | ||
return 'nav-charts'; | ||
} | ||
return ''; | ||
}; | ||
|
||
const ActivityTableRow: React.FC<ActivityProps> = ({ | ||
loading, | ||
activeChild, | ||
activityList, | ||
}) => ( | ||
<> | ||
{activityList.map(activity => ( | ||
<CardStyles | ||
key={`${activity.dashboard_title}__${activity.time}__${activity.id}`} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This solved the problem of "not unique" keys. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ktmud The "infinite cards bug" is a problem with keys (or lack thereof). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These types need a better definition. Several "mandatory" fields are present only sometimes (example, id) ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ktmud That's the problem, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: #13291 |
||
onClick={() => { | ||
window.location.href = getRecentRef(activeChild, activity); | ||
}} | ||
> | ||
<ListViewCard | ||
loading={loading} | ||
cover={<></>} | ||
url={ | ||
activity.sql | ||
? `/superset/sqllab?savedQueryId=${activity.id}` | ||
: activity.url | ||
} | ||
title={getFilterTitle(activity)} | ||
description={`Last Edited: ${moment( | ||
activity.changed_on_utc, | ||
'MM/DD/YYYY HH:mm:ss', | ||
)}`} | ||
avatar={getIconName(activity)} | ||
actions={null} | ||
/> | ||
</CardStyles> | ||
))} | ||
</> | ||
); | ||
export default ActivityTableRow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Does it makes sense to call this ActivityCard ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TableRow
sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename otherXxxTable
to simplyRecents
,Dashboards
,Charts
andSavedQueries
to be consistent with section titles, then the single tab content can be named asRecentsList
,DashboardList
, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.