Planning authoring-react fields #2147
Conversation
thecalcc
changed the base branch from
add-to-planning-refactoring
to
release/2.8
thecalcc
changed the base branch from
release/2.8
to
feature/multiple-events-in-planning
|
@tomaskikutis what's a better way to handle this? I have the branch based off of yours - Edit: might be a good idea for you to push your branch to the remote so I can merge to it as well, while not messing directly with it |
tomaskikutis
changed the base branch from
feature/multiple-events-in-planning
to
authoring-react-planning
|
I've deleted the branch from my fork and pushed it to this repo. You'll have to fix a conflict now since I've changed the content profile file. |
|
why are you removing package-lock from planning extension? |
Unintentionally, I will revert it |
|
Usually if I leave some comments I expect you would only push next after you handled it which seems not to be the case here right? |
| "resolved": "https://registry.npmjs.org/acorn/-/acorn-3.3.0.tgz", | ||
| "integrity": "sha512-OLUyIIZ7mF5oaAUT1w0TFqQS81q3saT46x8t7ukpPjMNk+nbs4ZHhs7ToV8EWnLYLepjETXd4XaCE4uxkMeqUw==", | ||
| "dev": true | ||
| "name": "superdesk-planning-extension", |
There was a problem hiding this comment.
no changes should be present in the PR of
client/planning-extension/package-lock.json
if you revert by copying over manually - use an editor that doesn't modify formatting
| const customVocabularyIds = | ||
| (planningProfile.schema?.['custom_vocabularies'] as IProfileSchemaTypeList)?.vocabularies; | ||
|
|
||
| if ((customVocabularyIds?.length ?? 0) > 0) { |
There was a problem hiding this comment.
custom vocabularies definition is quite big, could you move it to a separate file? here you would import the function, call it and push the result to the array of fields
| @@ -15,7 +16,18 @@ export const storageAdapterPlanningItem: IStorageAdapter<IPlanningItem> = { | |||
| const fieldDefinitions = getFieldDefinitions(); | |||
| const fieldStorageAdapter = fieldDefinitions[fieldId]?.storageAdapter; | |||
|
|
|||
| if (fieldStorageAdapter != null) { | |||
| if (fieldId.includes(SUBJECT_PREFIX_ID)) { | |||
There was a problem hiding this comment.
use .startsWith instead of .includes
Regarding SUBJECT_PREFIX_ID - would be good if we could avoid this approach of checking contents of an ID. I suggested the idea to assure you that it can be done using existing vocabulary fields from client-core. Since we agree now that it can be done, we can look into how to do it better :) It's a bit messy that you run storage adapter twice for vocabulary fields. Once in field definition and second time in main storage adapter. Change IFieldDefinition['storageAdapter'] so you can do it all in once place.
| const fieldDefinitions = getFieldDefinitions(); | ||
| const fieldStorageAdapter = fieldDefinitions[fieldId]?.storageAdapter; | ||
|
|
||
| if (fieldStorageAdapter != null) { | ||
| if (fieldId.startsWith(SUBJECT_PREFIX_ID)) { |
There was a problem hiding this comment.
use the same approach here as well to avoid running 2 storage adapters
client/components/planning-editor-standalone/field-adapters/custom-vocabularies.ts
Outdated
Show resolved
Hide resolved
client/components/planning-editor-standalone/profile-converter.ts
Outdated
Show resolved
Hide resolved
| storageAdapter: { | ||
| storeValue: (item, operationalValue: IVocabularyItem['qcode']) => { | ||
| const vocabulary = allVocabularies.get(id); | ||
| const vocabItems = vocabulary.items.filter((x) => operationalValue.includes(x.qcode)) ?? []; |
There was a problem hiding this comment.
Default will never get applied here. If .filter does not crash it will always return an array.
| import { | ||
| IAttachmentsFieldConfig, | ||
| } from '../../planning-extension/src/authoring-react-fields/planning-attachments/interfaces'; | ||
| import {getCustomVocabularyFields} from './field-adapters/custom-vocabularies'; | ||
| import {getPlanningProfileFields} from './profile-fields'; | ||
| import {IEventOrPlanningItem} from 'interfaces'; |
There was a problem hiding this comment.
use only planning item interface here. Event will have a separate editor
|
FYI it was pretty annoying to have the date PR together with vocabularies PR. Since you was refactoring a lot in this PR I would never review diff between commits, but a diff between target branch and this branch - so I saw how final changes look. I then had to look at date field multiple times when there were no changes there. Next time spin up another branch from the PR branch to separate work on different features. |
tomaskikutis
merged commit d0cd58a
into
superdesk:authoring-react-planning
STT-63
Front-end checklist
memoorPureComponentto define new React components (and updates existing usages in modified code segments)lodash.getwith optional chaining and nullish coalescing for modified code segmentssuperdeskApi)superdesk-ui-frameworkandsuperdeskApiwhen possible instead of using ones defined in this repository.planningApiwhere it is possible to usesuperdeskApiplanningApi)