feat(react-components): functionality surrounding Points of interest #4866
Conversation
There was a problem hiding this comment.
Nice work!
Some comments that involves some discussions and the request for changes are more about to check if does make sense such as the multiple comments on PoIs (how to get a single comment from a single PoI, for example).
If the concerns doesnt make sense or the code is supporting the concern already, then the rest are not a blocker but more some improvements on top of the current version, I think.
|
|
||
| public override getPlaceholder(t: TranslateDelegate): string | undefined { | ||
| return t('COMMENT_PLACEHOLDER', 'Write a comment'); | ||
| } |
There was a problem hiding this comment.
I was wondering if it would be better to have the translator instance as a property of the base class (and instantiated in the constructor, perhaps) instead of passing through it as a parameter and flying around on different methods everywhere risking to complicated to be GCollected if some ref was created in other places out of the class instance. If it is a class parameter and used internally only, then it would be Gcollected when the instance is removed.
But ofc, maybe I'm overthinking, or shouldn't work as expected.
And I know this is part of the architecture so we don't have to change that now (if does make sense, ofc) ... I'm just raising this reasoning 🤔
There was a problem hiding this comment.
Yeah, it's part of the architecture, so we'd probably need to convince Nils Petter 😅 But I think there is only one Translation instance in practice, so not sure Garbage Collection would be a problem anyway? 🤔
...s/src/architecture/concrete/pointsOfInterest/CreatePointsOfInterestWithDescriptionCommand.ts
Show resolved
Hide resolved
| } | ||
|
|
||
| public override get tooltip(): TranslateKey { | ||
| return { key: 'CREATE_POI', fallback: 'Create point of interest' }; |
There was a problem hiding this comment.
So here we are using the translate key as the returning type (assuming that it will be used by the translator outside in some react component, right?.
But in other methods we are returning the string directly by processing the translator within the method by getting the TranslateDelegate argument. Is there a good or easy way to keep the same pattern or doesnt make sense in this case? 🤔
| constructor(poiProvider: PointsOfInterestProvider<PoIId>) { | ||
| this._poiProvider = poiProvider; | ||
| this._loadedPromise = poiProvider.fetchAllPointsOfInterest(); | ||
| } | ||
|
|
||
| public async getPoiCommentsForPoi(id: PoIId): Promise<CommentProperties[]> { |
There was a problem hiding this comment.
is it not better PoiID(or PoiId) instead of PoIID or PoIId ? Hard to read 😆
| import { Quantity } from '../../base/domainObjectsHelpers/Quantity'; | ||
| import { type PointsOfInterestProvider } from './PointsOfInterestProvider'; | ||
| import { isDefined } from '../../../utilities/isDefined'; | ||
|
|
||
| export class PointsOfInterestDomainObject<PoIIdType> extends VisualDomainObject { | ||
| private _selectedPointsOfInterest: PointsOfInterest<PoIIdType> | undefined; | ||
| private _selectedPointsOfInterest: PointOfInterest<PoIIdType> | undefined; |
There was a problem hiding this comment.
PoiIdType perhaps? No? 😆
| poiObject | ||
| }: { | ||
| poiObject: PointsOfInterestDomainObject<any>; | ||
| }): ReactElement => { |
There was a problem hiding this comment.
Is it not better to use ReactNode here as well even not having any the other returning for now?
| domainObject: PointsOfInterestDomainObject<unknown>, | ||
| poi: PointOfInterest<unknown> | ||
| ): UseQueryResult<CommentProperties[]> { | ||
| return useQuery({ |
There was a problem hiding this comment.
In other parts of the code (Search UI, for instance) whenever we use the useQuery hook, we are moving the hooks to a subfolder called query and adding a sufix, something like: useCommentsForPoIQuery.
Are we gonna follow the same pattern here?
| import { useRenderTarget } from '../../RevealCanvas'; | ||
| import { useOnUpdate } from '../useOnUpdate'; | ||
|
|
||
| export function usePoIDomainObject(): PointsOfInterestDomainObject<any> | undefined { |
There was a problem hiding this comment.
is there a way to get rid of the any here?
| export const useSyncSelectedPoI = ( | ||
| selectedPoi: PointOfInterest<any> | undefined, | ||
| setSelectedPoi: (poi: PointOfInterest<any> | undefined) => void | ||
| ) => { |
There was a problem hiding this comment.
Can we get rid of the any here as well or it would demand to much effort?
Btw, these "any"s seems like a temporary definition as we are planning to publish this feature asap, isnt? I think it is fine to keep it for now but maybe we can think about it once we have more time.
| @@ -7,6 +7,8 @@ export const queryKeys = { | |||
| all: ['cdf'] as const, | |||
| // ASSETS | |||
| assetsById: (ids: IdEither[]) => [...assets, ids] as const, | |||
| // Points of interest | |||
| poiCommentsById: (id: any) => [...pois, id] as const, | |||
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/
Description 📝
Contains
PoIListshows all loaded PoIsPoIInfoPanelContentshows more details about a single point of interestHow has this been tested? 🔍
Test instructions ℹ️
Checklist ☑️